From 14d4eabd7f7c76f321671c42f9a5d9bcee12559d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 6 Feb 2026 06:50:35 +0000 Subject: [PATCH 1/3] Add principal-level code review for readability, simplicity, and redundancy Reviews the full codebase with focus on eliminating duplication, improving readability, and simplifying unnecessary complexity. Key findings include ~70% duplicated logic across the three view components, duplicate type definitions, copy-pasted utility functions, and dead code paths. https://claude.ai/code/session_01UugExn4ppZVxP8SG576CeP --- CODE_REVIEW.md | 235 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 235 insertions(+) create mode 100644 CODE_REVIEW.md diff --git a/CODE_REVIEW.md b/CODE_REVIEW.md new file mode 100644 index 0000000..1a8072c --- /dev/null +++ b/CODE_REVIEW.md @@ -0,0 +1,235 @@ +# Code Review: GitVegas — Readability, Simplicity, Redundancy + +Reviewed as a principal developer focusing on readability, simplicity, and eliminating redundancy. + +--- + +## CRITICAL: Duplicated Logic Across Views + +The most significant problem in this codebase is that **IssuesAndPRsList**, **SummaryView**, and **EventView** are ~70% identical. Each independently implements: + +- Selection state (`toggleItemSelection`, `clearSelection`, `bulkSelectItems`, `selectAllItems`) +- Select-all checkbox state computation (`selectAllState` useMemo) +- `handleSelectAllChange` handler +- `copyResultsToClipboard` with nearly identical clipboard formatting +- `isClipboardCopied` wrapper (a trivial pass-through to `isCopied`) +- Dialog state + navigation (`selectedItemForDialog`, `handlePreviousItem`, `handleNextItem`, `getCurrentItemIndex`) +- Empty state logic (`hasRawData`/`hasRawEvents`, `hasSearchText`) + +**Files**: `src/views/IssuesAndPRsList.tsx:86-520`, `src/views/Summary.tsx:44-457`, `src/views/EventView.tsx:32-305` + +**Recommendation**: Extract a `useListSelection` hook and a `useDialogNavigation` hook. These three views should share the behavior, not copy-paste it. Rough sketch: + +```ts +// useListSelection(items: GitHubItem[]) +// returns: selectedItems, toggleItem, selectAll, clearSelection, bulkSelect, selectAllState +``` + +```ts +// useDialogNavigation(items: GitHubItem[]) +// returns: selectedItem, setSelectedItem, handlePrev, handleNext, hasPrev, hasNext +``` + +--- + +## HIGH: Redundant `isClipboardCopied` Wrappers + +All three views define: + +```ts +const isClipboardCopied = useCallback((itemId: string | number) => { + return isCopied(itemId); +}, [isCopied]); +``` + +**Files**: `IssuesAndPRsList.tsx:266-268`, `Summary.tsx:174-176`, `EventView.tsx:116-118` + +This wraps `isCopied` in `useCallback` for no benefit -- `isCopied` is already a stable callback from `useCopyFeedback`. Just pass `isCopied` directly. + +--- + +## HIGH: Duplicated Utility Functions + +### `parseUsernames` / `parseCommaSeparatedUsernames` + +- `src/utils/summaryGrouping.ts:14-16` defines `parseUsernames` +- `src/utils/viewFiltering.ts:133-135` defines `parseCommaSeparatedUsernames` + +These are identical functions. Keep one, delete the other. + +### `isAuthoredBySearchedUser` / `isItemAuthoredBySearchedUsers` + +- `src/utils/summaryGrouping.ts:21-24` defines `isAuthoredBySearchedUser` +- `src/utils/viewFiltering.ts:140-144` defines `isItemAuthoredBySearchedUsers` + +Same function, different names. The `viewFiltering` version unnecessarily re-lowercases the array on every call (should do it once outside). Keep one. + +### `categorizeItem` / `categorizeItemWithoutDateFiltering` + +- `src/utils/summaryGrouping.ts:109-180` vs `src/utils/summaryGrouping.ts:185-274` + +These two functions share ~80% of their logic (review dedup, comment handling, commit, other, PR categorization). The only difference is whether `updatedInRange` is checked. They should be a single function with a parameter, not two separate 90-line functions. + +--- + +## HIGH: `FormContextType` Defined Twice + +- `src/App.tsx:34-59` defines a local `FormContextType` +- `src/types.ts:184-206` exports a nearly identical `FormContextType` + +The App.tsx version has extra fields (`searchText`, `setSearchText`, `searchItemsCount`, `eventsCount`, `rawEventsCount`). This is confusing -- one file exports a type that is never used for the actual context, while the actual context type is hidden in App.tsx. Consolidate into `types.ts`. + +--- + +## HIGH: ItemRow Desktop/Mobile Duplication + +`src/components/ItemRow.tsx` renders the **entire component twice**: once for desktop (lines 67-252) and once for mobile (lines 255-457). The icon logic, avatar logic, title/link rendering, and action buttons are fully duplicated using CSS `display: none` toggling. + +This doubles the DOM size for every row and makes maintenance error-prone (fix a bug in one layout, forget the other). + +**Recommendation**: Use a single layout with responsive CSS, or extract shared sub-components (`StatusIcon`, `AvatarGroup`, `TitleLink`) used by both layouts. + +--- + +## MEDIUM: Dead/Unused Code + +### `handleUsernameBlur` is empty + +`src/hooks/useGitHubFormState.ts:230-233`: +```ts +const handleUsernameBlur = useCallback(async () => { + // This can be used for additional validation if needed +}, []); +``` +An empty async callback that is threaded through the context and called in `SearchForm.tsx`. Remove it or implement it. + +### `showUser` prop accepted but unused + +`src/components/ItemRow.tsx:28` declares `showUser?: boolean` but it's never referenced in the component body. It's always passed as `showUser={true}` by callers. Dead prop. + +### Unreachable code in `useGitHubDataFetching` + +`src/hooks/useGitHubDataFetching.ts:113-115`: +```ts +throw error; +// Continue with what we have so far +hasMorePages = false; +``` +Lines 114-115 are unreachable after `throw`. The comment suggests the intent was to **not** throw. + +### `buttonStyles` export from App.tsx + +`src/App.tsx:73-80` exports a `buttonStyles` object that is only used by `IssuesAndPRsList`. It's passed as a prop through the component tree. This should live closer to where it's used, or in a shared styles file. An App-level export of style constants is odd. + +--- + +## MEDIUM: Redundant Length Check in `utils.ts` + +`src/utils.ts:82-91`: +```ts +if (trimmed.length === 0) { + return { isValid: false, error: 'Username cannot be empty' }; +} +if (trimmed.length < 1) { + return { isValid: false, error: 'Username must be at least 1 character long' }; +} +``` + +`length === 0` and `length < 1` are the same condition. The second branch is unreachable. + +--- + +## MEDIUM: Overly Complex Avatar URL Memo + +`src/App.tsx:157-164`: +```ts +const avatarUrls = useMemo(() => { + if (cachedAvatarUrls && cachedAvatarUrls.length > 0) { + return cachedAvatarUrls; + } + return []; +}, [cachedAvatarUrls]); +``` + +This memo does nothing useful. `cachedAvatarUrls` is already a memoized value from `useGitHubFormState`. At most this is `cachedAvatarUrls ?? []`, and the hook already returns `[]` as default. Remove the memo entirely. + +--- + +## MEDIUM: `as unknown as` Type Assertions + +The codebase has multiple `as unknown as` casts that indicate a type mismatch that should be fixed at the source: + +- `App.tsx:513-515`: `indexedDBSearchItems as unknown as GitHubItem[]` +- `useGitHubDataFetching.ts:292`: `sortedSearchItems as unknown as GitHubEvent[]` +- `useGitHubDataProcessing.ts:45`: `indexedDBSearchItems as unknown as GitHubItem[]` + +The `useIndexedDBStorage` hook returns `GitHubEvent[]` but search items are `GitHubItem[]`. Rather than casting everywhere, make the hook generic or create a separate hook for search items. + +--- + +## MEDIUM: `caseInsensitiveCompare` Defined Inside Component + +`src/App.tsx:167-168` defines a comparator function inside the `App` component body (not in a `useCallback` or `useMemo`). It gets recreated on every render. Since it's a pure function with no dependencies, move it outside the component. + +--- + +## LOW: Inconsistent Comment Artifacts + +Several files have blank lines where code was removed, leftover `// removed` comments, or empty blocks: + +- `Summary.tsx:171-172`: empty lines +- `Summary.tsx:224-228`: empty lines with orphaned comment `// Single item clipboard copy handler` +- `EventView.tsx:157-158`: empty lines +- `IssuesAndPRsList.tsx:316`: empty blank line +- `IssuesAndPRsList.tsx:93-94`: empty comment block + +These are noise. Clean them up. + +--- + +## LOW: `headerRight = null` Pattern + +Both `Summary.tsx:294` and `EventView.tsx:228` set `const headerRight = null` then pass it as a prop. This is unnecessary ceremony -- just omit the prop or pass `null` inline. + +--- + +## LOW: Test Environment Detection Duplicated + +`src/App.tsx:220-223` and `src/hooks/useLocalStorage.ts:132-135` both duplicate the same test-environment detection logic: +```ts +const isTestEnvironment = typeof window !== 'undefined' && + (window.navigator?.userAgent?.includes('jsdom') || + process.env.NODE_ENV === 'test' || + import.meta.env?.MODE === 'test'); +``` + +Extract to a utility: `isTestEnvironment()`. + +--- + +## LOW: `summaryHelpers.ts` Over-Extraction + +`src/utils/summaryHelpers.ts` contains tiny one-liner functions: +```ts +export const hasAnyItems = (groups) => Object.values(groups).some(items => items.length > 0); +export const isSectionCollapsed = (name, set) => set.has(name); +``` + +`isSectionCollapsed` is literally `Set.has` with a wrapper. `hasAnyItems` is one expression. These add indirection without value. Inline them at the call site. + +--- + +## Summary of Recommendations (by impact) + +| Priority | Issue | Action | +|----------|-------|--------| +| **Critical** | 3 views duplicate selection, dialog, clipboard logic | Extract `useListSelection` and `useDialogNavigation` hooks | +| **High** | `categorizeItem` / `categorizeItemWithoutDateFiltering` near-identical | Merge into one parameterized function | +| **High** | `FormContextType` defined in two places | Consolidate in `types.ts` | +| **High** | ItemRow renders full desktop + mobile DOM | Share sub-components or use responsive CSS | +| **High** | Duplicate utility functions across files | Delete duplicates (`parseUsernames`, `isAuthoredBy...`) | +| **Medium** | `as unknown as` casts for IndexedDB items | Make `useIndexedDBStorage` generic | +| **Medium** | Unreachable code after `throw` | Remove dead lines or fix intent | +| **Medium** | Empty `handleUsernameBlur` | Remove or implement | +| **Medium** | Redundant `length === 0` / `length < 1` checks | Remove duplicate check | +| **Low** | Blank lines, orphan comments, `headerRight = null` | Clean up | From 0c9f7daecdb624751e4208d4fed3467d49837bf5 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 6 Feb 2026 07:52:06 +0000 Subject: [PATCH 2/3] Refactor: eliminate duplication, dead code, and redundancy across codebase Major changes: - Extract useListSelection and useDialogNavigation hooks to deduplicate ~70% of shared logic across IssuesAndPRsList, Summary, and EventView - Merge categorizeItem and categorizeItemWithoutDateFiltering into one parameterized function (applyDateFiltering flag) - Remove empty handleUsernameBlur callback and all its references - Remove dead showUser prop from ItemRow - Remove redundant avatarUrls useMemo wrapper in App.tsx - Remove duplicate buttonStyles export from App.tsx - Move caseInsensitiveCompare outside component to avoid re-creation - Remove redundant length===0 / length<1 duplicate check in utils.ts - Extract isTestEnvironment() utility to eliminate duplication - Remove duplicate parseCommaSeparatedUsernames and isItemAuthoredBySearchedUsers from viewFiltering.ts - Remove duplicate FormContextType handleUsernameBlur from types.ts - Update all test files to match new interfaces All 641 tests pass (43 test files, 13 skipped). https://claude.ai/code/session_01UugExn4ppZVxP8SG576CeP --- src/App.test.tsx | 1 - src/App.tsx | 119 ++--- src/components/ItemRow.tsx | 1 - src/components/SearchForm.test.tsx | 11 - src/components/SearchForm.tsx | 34 +- .../SearchForm-RaceCondition.test.tsx | 2 - src/components/__tests__/Summary.test.tsx | 1 - src/hooks/useDialogNavigation.ts | 46 ++ src/hooks/useGitHubFormState.test.tsx | 1 - src/hooks/useGitHubFormState.ts | 8 - src/hooks/useListSelection.ts | 84 +++ src/hooks/useLocalStorage.ts | 9 +- src/test/setup.ts | 1 - src/test/test-utils.tsx | 1 - src/types.ts | 1 - src/utils.ts | 7 - src/utils/__tests__/viewFiltering.test.ts | 45 -- src/utils/environment.ts | 8 + src/utils/summaryGrouping.ts | 242 ++------- src/utils/viewFiltering.ts | 16 +- src/views/EventView.tsx | 236 +++------ src/views/IssuesAndPRsList.tsx | 480 ++++++------------ src/views/Summary.tsx | 323 +++--------- src/views/__tests__/IssuesAndPRsList.test.tsx | 2 +- 24 files changed, 515 insertions(+), 1164 deletions(-) create mode 100644 src/hooks/useDialogNavigation.ts create mode 100644 src/hooks/useListSelection.ts create mode 100644 src/utils/environment.ts diff --git a/src/App.test.tsx b/src/App.test.tsx index 1eef7fa..efb1ab9 100644 --- a/src/App.test.tsx +++ b/src/App.test.tsx @@ -47,7 +47,6 @@ vi.mock('./hooks/useGitHubFormState', () => ({ setEndDate: vi.fn(), setGithubToken: vi.fn(), setApiMode: vi.fn(), - handleUsernameBlur: vi.fn(), validateUsernameFormat: vi.fn(), error: null, setError: vi.fn(), diff --git a/src/App.tsx b/src/App.tsx index 64d11ab..312233b 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -1,6 +1,6 @@ import { useState, - useEffect, + useEffect, useCallback, useMemo, createContext, @@ -15,6 +15,7 @@ import { useGitHubDataFetching } from './hooks/useGitHubDataFetching'; import { useGitHubDataProcessing } from './hooks/useGitHubDataProcessing'; import { useIndexedDBStorage } from './hooks/useIndexedDBStorage'; import { GitHubItem } from './types'; +import { isTestEnvironment } from './utils/environment'; import SearchForm from './components/SearchForm'; import IssuesAndPRsList from './views/IssuesAndPRsList'; @@ -42,12 +43,9 @@ interface FormContextType { setStartDate: (date: string) => void; setEndDate: (date: string) => void; setGithubToken: (token: string) => void; - setApiMode: ( - mode: 'search' | 'events' | 'summary' - ) => void; + setApiMode: (mode: 'search' | 'events' | 'summary') => void; setSearchText: (searchText: string) => void; handleSearch: () => void; - handleUsernameBlur: () => void; validateUsernameFormat: (username: string) => void; addAvatarsToCache: (avatarUrls: { [username: string]: string }) => void; loading: boolean; @@ -69,15 +67,9 @@ export function useFormContext() { return context; } -// eslint-disable-next-line react-refresh/only-export-components -export const buttonStyles = { - height: 28, - minWidth: 0, - display: 'inline-flex', - alignItems: 'center', - justifyContent: 'center', - gap: 2, -}; +// Pure comparator for case-insensitive string sorting (outside component to avoid re-creation) +const caseInsensitiveCompare = (a: string, b: string): number => + a.toLowerCase().localeCompare(b.toLowerCase()); function App() { const [isSettingsOpen, setIsSettingsOpen] = useState(false); @@ -97,7 +89,6 @@ function App() { setEndDate, setGithubToken, setApiMode, - handleUsernameBlur, validateUsernameFormat, addAvatarsToCache, error, @@ -154,19 +145,6 @@ function App() { clearSearchItems, }); - const avatarUrls = useMemo(() => { - // Prioritize cached avatar URLs if they exist, otherwise return empty array - // Only return non-empty arrays to prevent unnecessary re-renders - if (cachedAvatarUrls && cachedAvatarUrls.length > 0) { - return cachedAvatarUrls; - } - return []; - }, [cachedAvatarUrls]); - - // Pure comparator for case-insensitive string sorting - const caseInsensitiveCompare = (a: string, b: string): number => - a.toLowerCase().localeCompare(b.toLowerCase()); - // Extract unique users from results for search suggestions (with avatars) const availableUsers = useMemo(() => { const seen = new Set(); @@ -185,7 +163,6 @@ function App() { // Extract labels from results sorted by frequency (most used first) const availableLabels = useMemo(() => { - // Build frequency map using reduce (functional approach) const labelCounts = results .flatMap(item => item.labels?.map(label => label.name) ?? []) .reduce((acc, label) => { @@ -193,7 +170,6 @@ function App() { return acc; }, new Map()); - // Sort by frequency (descending), then alphabetically for ties return Array.from(labelCounts.entries()) .sort((a, b) => b[1] - a[1] || caseInsensitiveCompare(a[0], b[0])) .map(([label]) => label); @@ -216,27 +192,18 @@ function App() { useEffect(() => { if (initialLoadingCount === 1) { - // Skip initial loading in test environment - const isTestEnvironment = typeof window !== 'undefined' && - (window.navigator?.userAgent?.includes('jsdom') || - process.env.NODE_ENV === 'test' || - import.meta.env?.MODE === 'test'); - - if (isTestEnvironment) { - // In tests, immediately exit loading mode + if (isTestEnvironment()) { setInitialLoadingCount(0); setIsDataLoadingComplete(false); return; } - + const startTime = Date.now(); - const minLoadingTime = 3000; // 3 seconds minimum for better UX - + const minLoadingTime = 3000; + handleSearch().then(() => { const elapsedTime = Date.now() - startTime; const remainingTime = Math.max(0, minLoadingTime - elapsedTime); - - // Instead of automatically switching, mark data loading as complete setTimeout(() => { setIsDataLoadingComplete(true); }, remainingTime); @@ -266,7 +233,6 @@ function App() { }, }} > - {/* GitVegas Branding - Vertical Layout */} - {/* GitVegas Title */} GitVegas - - {/* Slot Machine */} - + + - - {/* Start Button */} + - - {/* Messages - Fixed height to prevent layout jumping */} - GitVegas - {/* Loading indicator - always visible */} - - {/* Header search */} + - - {/* Mobile-optimized actions */} + :first-child': { display: 'none' }, }, }} > - + setIsSettingsOpen(true)} /> - - {/* Slot machine - smaller on mobile */} + - ) : ( - + )} - {/* PWA Update Notification */} ); diff --git a/src/components/ItemRow.tsx b/src/components/ItemRow.tsx index 3e98b89..d3308f3 100644 --- a/src/components/ItemRow.tsx +++ b/src/components/ItemRow.tsx @@ -25,7 +25,6 @@ interface ItemRowProps { onSelect?: (id: string | number) => void; showCheckbox?: boolean; showRepo?: boolean; - showUser?: boolean; showTime?: boolean; size?: 'small' | 'medium'; groupCount?: number; diff --git a/src/components/SearchForm.test.tsx b/src/components/SearchForm.test.tsx index a7f3e42..adc9459 100644 --- a/src/components/SearchForm.test.tsx +++ b/src/components/SearchForm.test.tsx @@ -24,7 +24,6 @@ const mockSetStartDate = vi.fn(); const mockSetEndDate = vi.fn(); const mockSetApiMode = vi.fn(); const mockHandleSearch = vi.fn(); -const mockHandleUsernameBlur = vi.fn(); const mockValidateUsernameFormat = vi.fn(); const mockAddAvatarsToCache = vi.fn(); @@ -39,7 +38,6 @@ vi.mock('../App', () => ({ apiMode: 'events' as const, setApiMode: mockSetApiMode, handleSearch: mockHandleSearch, - handleUsernameBlur: mockHandleUsernameBlur, validateUsernameFormat: mockValidateUsernameFormat, addAvatarsToCache: mockAddAvatarsToCache, loading: false, @@ -125,15 +123,6 @@ describe('SearchForm', () => { }); }); - it('calls handleUsernameBlur on blur event', () => { - render(); - const usernameInput = screen.getByPlaceholderText(/Enter usernames/); - - fireEvent.blur(usernameInput); - - expect(mockHandleUsernameBlur).toHaveBeenCalledTimes(1); - }); - it('calls handleSearch on form submit', () => { render(); const updateButton = screen.getByText('Update'); diff --git a/src/components/SearchForm.tsx b/src/components/SearchForm.tsx index 3619482..312c6df 100644 --- a/src/components/SearchForm.tsx +++ b/src/components/SearchForm.tsx @@ -21,7 +21,6 @@ const SearchForm = memo(function SearchForm() { apiMode, setApiMode, handleSearch, - handleUsernameBlur, validateUsernameFormat, addAvatarsToCache, loading, @@ -48,16 +47,12 @@ const SearchForm = memo(function SearchForm() { const handleUsernameBlurEvent = useCallback( async (e: React.FocusEvent) => { const username = e.target.value.trim(); - if (!username) { - handleUsernameBlur(); - return; - } + if (!username) return; // First validate format const validation = validateUsernameList(username); if (validation.errors.length > 0) { - validateUsernameFormat(username); // This will set the error - handleUsernameBlur(); + validateUsernameFormat(username); return; } @@ -70,38 +65,29 @@ const SearchForm = memo(function SearchForm() { ); if (result.invalid.length > 0) { - const invalidUsernames = result.invalid; - const errorMessages = invalidUsernames.map(username => { - const errorMsg = result.errors[username] || 'Username validation failed'; - return `${username}: ${errorMsg}`; + const errorMessages = result.invalid.map(u => { + const errorMsg = result.errors[u] || 'Username validation failed'; + return `${u}: ${errorMsg}`; }); - - // Set API validation error - use a simpler approach - const errorMessage = `Invalid GitHub username${invalidUsernames.length > 1 ? 's' : ''}:\n${errorMessages.join('\n')}`; - validateUsernameFormat(errorMessage); // Just pass the error message + validateUsernameFormat( + `Invalid GitHub username${result.invalid.length > 1 ? 's' : ''}:\n${errorMessages.join('\n')}` + ); } else { - // All usernames are valid validateUsernameFormat(''); // Clear any errors - // Save avatars to cache for slot machine display if (Object.keys(result.avatarUrls).length > 0) { addAvatarsToCache(result.avatarUrls); } - // Save to localStorage only if validation passes localStorage.setItem('github-username', username); } } catch (error) { console.warn('GitHub API validation failed:', error); - // Don't block form submission for network errors - just log them - validateUsernameFormat(''); // Clear errors for network issues - // Still save to localStorage for format-valid usernames + validateUsernameFormat(''); localStorage.setItem('github-username', username); } finally { setIsValidating(false); } - - handleUsernameBlur(); }, - [validateUsernameFormat, handleUsernameBlur, githubToken] + [validateUsernameFormat, githubToken, addAvatarsToCache] ); // Effect to handle pending search after validation completes diff --git a/src/components/__tests__/SearchForm-RaceCondition.test.tsx b/src/components/__tests__/SearchForm-RaceCondition.test.tsx index 99f09c5..ba74475 100644 --- a/src/components/__tests__/SearchForm-RaceCondition.test.tsx +++ b/src/components/__tests__/SearchForm-RaceCondition.test.tsx @@ -5,7 +5,6 @@ import SearchForm from '../SearchForm'; // Mock the form context const mockHandleSearch = vi.fn(); -const mockHandleUsernameBlur = vi.fn(); const mockValidateUsernameFormat = vi.fn(); const mockAddAvatarsToCache = vi.fn(); const mockSetUsername = vi.fn(); @@ -38,7 +37,6 @@ vi.mock('../../App', () => ({ apiMode: 'summary', setApiMode: mockSetApiMode, handleSearch: mockHandleSearch, - handleUsernameBlur: mockHandleUsernameBlur, validateUsernameFormat: mockValidateUsernameFormat, addAvatarsToCache: mockAddAvatarsToCache, loading: false, diff --git a/src/components/__tests__/Summary.test.tsx b/src/components/__tests__/Summary.test.tsx index 2a42179..c9aec7a 100644 --- a/src/components/__tests__/Summary.test.tsx +++ b/src/components/__tests__/Summary.test.tsx @@ -17,7 +17,6 @@ const mockFormContext = { setGithubToken: vi.fn(), setApiMode: vi.fn(), handleSearch: vi.fn(), - handleUsernameBlur: vi.fn(), validateUsernameFormat: vi.fn(), loading: false, loadingProgress: '', diff --git a/src/hooks/useDialogNavigation.ts b/src/hooks/useDialogNavigation.ts new file mode 100644 index 0000000..a2af479 --- /dev/null +++ b/src/hooks/useDialogNavigation.ts @@ -0,0 +1,46 @@ +import { useState, useCallback } from 'react'; +import { GitHubItem } from '../types'; + +interface UseDialogNavigationReturn { + selectedItemForDialog: GitHubItem | null; + setSelectedItemForDialog: (item: GitHubItem | null) => void; + handlePreviousItem: () => void; + handleNextItem: () => void; + hasPrevious: boolean; + hasNext: boolean; +} + +/** + * Shared hook for dialog navigation (prev/next) used across all view components. + */ +export function useDialogNavigation( + items: GitHubItem[], +): UseDialogNavigationReturn { + const [selectedItemForDialog, setSelectedItemForDialog] = + useState(null); + + const currentIndex = selectedItemForDialog + ? items.findIndex(item => item.id === selectedItemForDialog.id) + : -1; + + const handlePreviousItem = useCallback(() => { + if (currentIndex > 0) { + setSelectedItemForDialog(items[currentIndex - 1]); + } + }, [currentIndex, items]); + + const handleNextItem = useCallback(() => { + if (currentIndex >= 0 && currentIndex < items.length - 1) { + setSelectedItemForDialog(items[currentIndex + 1]); + } + }, [currentIndex, items]); + + return { + selectedItemForDialog, + setSelectedItemForDialog, + handlePreviousItem, + handleNextItem, + hasPrevious: currentIndex > 0, + hasNext: currentIndex >= 0 && currentIndex < items.length - 1, + }; +} diff --git a/src/hooks/useGitHubFormState.test.tsx b/src/hooks/useGitHubFormState.test.tsx index 55d287d..5d85dab 100644 --- a/src/hooks/useGitHubFormState.test.tsx +++ b/src/hooks/useGitHubFormState.test.tsx @@ -114,7 +114,6 @@ describe('useGitHubFormState - URL Parameter Processing', () => { expect(result.current).toHaveProperty('setEndDate'); expect(result.current).toHaveProperty('setGithubToken'); expect(result.current).toHaveProperty('setApiMode'); - expect(result.current).toHaveProperty('handleUsernameBlur'); expect(result.current).toHaveProperty('validateUsernameFormat'); expect(result.current).toHaveProperty('error'); expect(result.current).toHaveProperty('setError'); diff --git a/src/hooks/useGitHubFormState.ts b/src/hooks/useGitHubFormState.ts index 27d8ca0..f2f4282 100644 --- a/src/hooks/useGitHubFormState.ts +++ b/src/hooks/useGitHubFormState.ts @@ -20,7 +20,6 @@ interface UseGitHubFormStateReturn { setEndDate: (endDate: string) => void; setGithubToken: (githubToken: string) => void; setApiMode: (apiMode: 'search' | 'events' | 'summary') => void; - handleUsernameBlur: () => Promise; validateUsernameFormat: (username: string) => void; addAvatarsToCache: (avatarUrls: { [username: string]: string }) => void; error: string | null; @@ -226,12 +225,6 @@ export const useGitHubFormState = (onUrlParamsProcessed?: () => void): UseGitHub [] ); - // Handle username blur event - const handleUsernameBlur = useCallback(async () => { - // This can be used for additional validation if needed - // Currently, immediate validation is handled in the useEffect above - }, []); - return { formSettings, setFormSettings, @@ -240,7 +233,6 @@ export const useGitHubFormState = (onUrlParamsProcessed?: () => void): UseGitHub setEndDate, setGithubToken, setApiMode, - handleUsernameBlur, validateUsernameFormat, addAvatarsToCache, error, diff --git a/src/hooks/useListSelection.ts b/src/hooks/useListSelection.ts new file mode 100644 index 0000000..336a386 --- /dev/null +++ b/src/hooks/useListSelection.ts @@ -0,0 +1,84 @@ +import { useCallback, useMemo } from 'react'; +import { GitHubItem } from '../types'; +import { useLocalStorage } from './useLocalStorage'; + +interface UseListSelectionReturn { + selectedItems: Set; + toggleItemSelection: (id: string | number) => void; + selectAllItems: () => void; + clearSelection: () => void; + bulkSelectItems: (itemIds: (string | number)[], shouldSelect: boolean) => void; + selectAllState: { checked: boolean; indeterminate: boolean }; +} + +const getItemId = (item: GitHubItem): string | number => item.event_id || item.id; + +/** + * Shared hook for item selection logic used across all view components. + * Manages selected items state, select-all checkbox state, and bulk operations. + */ +export function useListSelection( + storageKey: string, + allSelectableItems: GitHubItem[], +): UseListSelectionReturn { + const [selectedItems, setSelectedItems] = useLocalStorage>(storageKey, new Set()); + + const toggleItemSelection = useCallback((id: string | number) => { + setSelectedItems(prev => { + const newSet = new Set(prev); + if (newSet.has(id)) { + newSet.delete(id); + } else { + newSet.add(id); + } + return newSet; + }); + }, []); + + const selectAllItems = useCallback(() => { + setSelectedItems(new Set(allSelectableItems.map(getItemId))); + }, [allSelectableItems]); + + const clearSelection = useCallback(() => { + setSelectedItems(new Set()); + }, []); + + const bulkSelectItems = useCallback((itemIds: (string | number)[], shouldSelect: boolean) => { + setSelectedItems(prev => { + const newSet = new Set(prev); + if (shouldSelect) { + itemIds.forEach(id => newSet.add(id)); + } else { + itemIds.forEach(id => newSet.delete(id)); + } + return newSet; + }); + }, []); + + const selectAllState = useMemo(() => { + if (allSelectableItems.length === 0) { + return { checked: false, indeterminate: false }; + } + + const selectedCount = allSelectableItems.filter(item => + selectedItems.has(getItemId(item)) + ).length; + + if (selectedCount === 0) { + return { checked: false, indeterminate: false }; + } else if (selectedCount === allSelectableItems.length) { + return { checked: true, indeterminate: false }; + } else { + return { checked: false, indeterminate: true }; + } + }, [allSelectableItems, selectedItems]); + + return { + selectedItems, + toggleItemSelection, + selectAllItems, + clearSelection, + bulkSelectItems, + selectAllState, + }; +} diff --git a/src/hooks/useLocalStorage.ts b/src/hooks/useLocalStorage.ts index 60a68ae..5f6010d 100644 --- a/src/hooks/useLocalStorage.ts +++ b/src/hooks/useLocalStorage.ts @@ -1,6 +1,7 @@ import { useState, useEffect, useCallback, useRef } from 'react'; import { getParamFromUrl, isValidDateString, validateUsernameList } from '../utils'; import { FormSettings } from '../types'; +import { isTestEnvironment } from '../utils/environment'; // Enhanced serialization that handles Set and Map objects function serializeValue(value: T): string { @@ -128,13 +129,7 @@ export function useFormSettings(key: string, initialValue: FormSettings, onUrlPa // Clear caches and data in background when loading with URL parameters (shared links) // This ensures fresh data for subsequent usage - // Only do this in production environment, not in tests - const isTestEnvironment = typeof window !== 'undefined' && - (window.navigator?.userAgent?.includes('jsdom') || - process.env.NODE_ENV === 'test' || - import.meta.env?.MODE === 'test'); - - if (!isTestEnvironment) { + if (!isTestEnvironment()) { // Run cache cleanup in background (don't await to avoid blocking the URL processing) (async () => { try { diff --git a/src/test/setup.ts b/src/test/setup.ts index 1183c14..2c76123 100644 --- a/src/test/setup.ts +++ b/src/test/setup.ts @@ -29,7 +29,6 @@ vi.mock('../App', async () => { setGithubToken: vi.fn(), setApiMode: vi.fn(), handleSearch: vi.fn(), - handleUsernameBlur: vi.fn(), validateUsernameFormat: vi.fn(), loading: false, loadingProgress: '', diff --git a/src/test/test-utils.tsx b/src/test/test-utils.tsx index 66ea771..fd59194 100644 --- a/src/test/test-utils.tsx +++ b/src/test/test-utils.tsx @@ -16,7 +16,6 @@ const mockFormContext: FormContextType = { setGithubToken: vi.fn(), setApiMode: vi.fn(), handleSearch: vi.fn(), - handleUsernameBlur: vi.fn(), validateUsernameFormat: vi.fn(), addAvatarsToCache: vi.fn(), loading: false, diff --git a/src/types.ts b/src/types.ts index a2062ee..d378b45 100644 --- a/src/types.ts +++ b/src/types.ts @@ -193,7 +193,6 @@ export interface FormContextType { setGithubToken: (value: string) => void; setApiMode: (value: 'search' | 'events' | 'summary') => void; handleSearch: (forceRefresh?: boolean) => void; - handleUsernameBlur: () => Promise; validateUsernameFormat: (username: string) => void; addAvatarsToCache: (avatarUrls: { [username: string]: string }) => void; loading: boolean; diff --git a/src/utils.ts b/src/utils.ts index 5ed0873..c35b72e 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -83,13 +83,6 @@ export const validateGitHubUsernameFormat = ( return { isValid: false, error: 'Username cannot be empty' }; } - if (trimmed.length < 1) { - return { - isValid: false, - error: 'Username must be at least 1 character long', - }; - } - if (trimmed.length > 39) { return { isValid: false, diff --git a/src/utils/__tests__/viewFiltering.test.ts b/src/utils/__tests__/viewFiltering.test.ts index 1bbdff2..7feea48 100644 --- a/src/utils/__tests__/viewFiltering.test.ts +++ b/src/utils/__tests__/viewFiltering.test.ts @@ -1,8 +1,6 @@ import { filterItemsByAdvancedSearch, sortItemsByUpdatedDate, - parseCommaSeparatedUsernames, - isItemAuthoredBySearchedUsers } from '../viewFiltering'; import { GitHubItem } from '../../types'; @@ -109,47 +107,4 @@ describe('viewFiltering utilities', () => { }); }); - describe('parseCommaSeparatedUsernames', () => { - it('should parse single username', () => { - const result = parseCommaSeparatedUsernames('alice'); - expect(result).toEqual(['alice']); - }); - - it('should parse multiple usernames', () => { - const result = parseCommaSeparatedUsernames('alice,bob,charlie'); - expect(result).toEqual(['alice', 'bob', 'charlie']); - }); - - it('should trim whitespace and convert to lowercase', () => { - const result = parseCommaSeparatedUsernames(' Alice , Bob , Charlie '); - expect(result).toEqual(['alice', 'bob', 'charlie']); - }); - - it('should handle empty strings', () => { - const result = parseCommaSeparatedUsernames(''); - expect(result).toEqual(['']); - }); - }); - - describe('isItemAuthoredBySearchedUsers', () => { - const item = createMockItem({ user: { login: 'Alice', avatar_url: '', html_url: '' } }); - - it('should return true when user is in searched usernames', () => { - const searchedUsers = ['alice', 'bob']; - const result = isItemAuthoredBySearchedUsers(item, searchedUsers); - expect(result).toBe(true); - }); - - it('should return false when user is not in searched usernames', () => { - const searchedUsers = ['bob', 'charlie']; - const result = isItemAuthoredBySearchedUsers(item, searchedUsers); - expect(result).toBe(false); - }); - - it('should be case insensitive', () => { - const searchedUsers = ['ALICE']; - const result = isItemAuthoredBySearchedUsers(item, searchedUsers); - expect(result).toBe(true); - }); - }); }); \ No newline at end of file diff --git a/src/utils/environment.ts b/src/utils/environment.ts new file mode 100644 index 0000000..21dadcb --- /dev/null +++ b/src/utils/environment.ts @@ -0,0 +1,8 @@ +/** + * Detects if the code is running in a test environment (jsdom, vitest, jest). + */ +export const isTestEnvironment = (): boolean => + typeof window !== 'undefined' && + (window.navigator?.userAgent?.includes('jsdom') || + process.env.NODE_ENV === 'test' || + import.meta.env?.MODE === 'test'); diff --git a/src/utils/summaryGrouping.ts b/src/utils/summaryGrouping.ts index ecf35a2..e6bb00c 100644 --- a/src/utils/summaryGrouping.ts +++ b/src/utils/summaryGrouping.ts @@ -8,21 +8,6 @@ export const getBasePRUrl = (htmlUrl: string): string => { return htmlUrl.split('#')[0]; }; -/** - * Parses usernames from a comma-separated string - */ -export const parseUsernames = (username: string): string[] => { - return username.split(',').map(u => u.trim().toLowerCase()); -}; - -/** - * Checks if an item is authored by any of the searched users - */ -export const isAuthoredBySearchedUser = (item: GitHubItem, searchedUsernames: string[]): boolean => { - const itemAuthor = item.user.login.toLowerCase(); - return searchedUsernames.includes(itemAuthor); -}; - /** * Determines the event type based on item properties using stable GitHub event types * Falls back to title parsing for items without originalEventType (e.g., from Search API) @@ -51,29 +36,14 @@ export const getEventType = (item: GitHubItem): string => { case 'GollumEvent': return 'other'; default: - // Unknown event type, fall back to title parsing break; } } - // Fallback to title parsing for items without originalEventType (e.g., from Search API) - // Check if this is a pull request review (title starts with "Review on:") - if (item.title.startsWith('Review on:')) { - return 'pull_request'; - } - // Check if this is a review comment (title starts with "Review comment on:") - if (item.title.startsWith('Review comment on:')) { - return 'comment'; - } - // Check if this is a comment event (title starts with "Comment on:") - if (item.title.startsWith('Comment on:')) { - return 'comment'; - } - // Check if this is a push event (title starts with "Committed") - if (item.title.startsWith('Committed')) { - return 'commit'; - } - // Check for other event types that don't belong to issues/PRs + if (item.title.startsWith('Review on:')) return 'pull_request'; + if (item.title.startsWith('Review comment on:')) return 'comment'; + if (item.title.startsWith('Comment on:')) return 'comment'; + if (item.title.startsWith('Committed')) return 'commit'; if ( item.title.startsWith('Created branch') || item.title.startsWith('Created tag') || @@ -98,180 +68,83 @@ export const isDateInRange = (dateStr: string, startDate: string, endDate: strin const date = new Date(dateStr); const start = new Date(startDate); const end = new Date(endDate); - // Add one day to end date to include the entire end day end.setDate(end.getDate() + 1); return date >= start && date < end; }; /** - * Categories a GitHub item without date filtering (for already date-filtered results) + * Categorizes a GitHub item based on its type and content. + * When applyDateFiltering is false (already date-filtered results), items that don't + * match created/merged/closed are placed in the "updated" bucket instead of being excluded. */ -export const categorizeItemWithoutDateFiltering = ( +export const categorizeItem = ( item: GitHubItem, addedReviewPRs: Set, startDate: string, - endDate: string + endDate: string, + applyDateFiltering = true, ): SummaryGroupName | null => { const type = getEventType(item); + // Review deduplication if (type === 'pull_request' && item.title?.startsWith('Review on:')) { const basePRUrl = getBasePRUrl(item.html_url); - const reviewKey = `${item.user.login}:${basePRUrl}`; // Deduplicate per person per PR + const reviewKey = `${item.user.login}:${basePRUrl}`; if (!addedReviewPRs.has(reviewKey)) { addedReviewPRs.add(reviewKey); return SUMMARY_GROUP_NAMES.PRS_REVIEWED; } - return null; // Skip duplicate reviews from same person on same PR - } - - if (type === 'comment' && item.title?.startsWith('Review comment on:')) { - // Review comments on PRs are ignored (section removed) return null; } - if (type === 'comment') { - return SUMMARY_GROUP_NAMES.COMMENTS; - } - - if (type === 'commit') { - return SUMMARY_GROUP_NAMES.COMMITS; - } - - if (type === 'other') { - return SUMMARY_GROUP_NAMES.OTHER_EVENTS; - } + if (type === 'comment' && item.title?.startsWith('Review comment on:')) return null; + if (type === 'comment') return SUMMARY_GROUP_NAMES.COMMENTS; + if (type === 'commit') return SUMMARY_GROUP_NAMES.COMMITS; + if (type === 'other') return SUMMARY_GROUP_NAMES.OTHER_EVENTS; if (type === 'pull_request') { - // Categorize PRs by their state and recent activity rather than strict date filtering const mergedAt = item.merged_at || item.pull_request?.merged_at; const createdInRange = isDateInRange(item.created_at, startDate, endDate); const mergedInRange = mergedAt && isDateInRange(mergedAt, startDate, endDate); const closedInRange = item.closed_at && isDateInRange(item.closed_at, startDate, endDate); - if (mergedAt && mergedInRange) { - return SUMMARY_GROUP_NAMES.PRS_MERGED; - } else if (item.state === 'closed' && closedInRange && !mergedAt) { - return SUMMARY_GROUP_NAMES.PRS_CLOSED; - } else if (createdInRange) { - return SUMMARY_GROUP_NAMES.PRS_OPENED; - } else { - // PR was updated but not created/merged/closed within timeframe - return SUMMARY_GROUP_NAMES.PRS_UPDATED; + if (mergedAt && mergedInRange) return SUMMARY_GROUP_NAMES.PRS_MERGED; + if (item.state === 'closed' && closedInRange && !mergedAt) return SUMMARY_GROUP_NAMES.PRS_CLOSED; + if (createdInRange) return SUMMARY_GROUP_NAMES.PRS_OPENED; + + if (applyDateFiltering) { + // Strict: only include if updated in range (and not already matched above) + const updatedInRange = isDateInRange(item.updated_at, startDate, endDate); + return updatedInRange ? SUMMARY_GROUP_NAMES.PRS_UPDATED : null; } + // Lenient: always fall back to "updated" for pre-filtered data + return SUMMARY_GROUP_NAMES.PRS_UPDATED; } - // Handle issues - categorize by recent activity regardless of authorship - // Ensure this is actually an issue and not a PR if (type === 'issue' && !item.pull_request) { const createdInRange = isDateInRange(item.created_at, startDate, endDate); const closedInRange = item.closed_at && isDateInRange(item.closed_at, startDate, endDate); - if (item.state === 'closed' && closedInRange) { - return SUMMARY_GROUP_NAMES.ISSUES_CLOSED; - } else if (createdInRange && (!item.action || item.action === 'opened')) { - // Issue was created in range - either explicitly opened or no action specified (from Search API) - return SUMMARY_GROUP_NAMES.ISSUES_OPENED; - } else { - return SUMMARY_GROUP_NAMES.ISSUES_UPDATED; + if (item.state === 'closed' && closedInRange) return SUMMARY_GROUP_NAMES.ISSUES_CLOSED; + if (createdInRange && (!item.action || item.action === 'opened')) return SUMMARY_GROUP_NAMES.ISSUES_OPENED; + + if (applyDateFiltering) { + const updatedInRange = isDateInRange(item.updated_at, startDate, endDate); + return updatedInRange ? SUMMARY_GROUP_NAMES.ISSUES_UPDATED : null; } + return SUMMARY_GROUP_NAMES.ISSUES_UPDATED; } return null; }; -/** - * Categories a GitHub item based on its type and content - */ -export const categorizeItem = ( +// Keep the old name as a re-export for backward compatibility with tests +export const categorizeItemWithoutDateFiltering = ( item: GitHubItem, addedReviewPRs: Set, startDate: string, - endDate: string -): SummaryGroupName | null => { - const type = getEventType(item); - - if (type === 'pull_request' && item.title?.startsWith('Review on:')) { - const basePRUrl = getBasePRUrl(item.html_url); - const reviewKey = `${item.user.login}:${basePRUrl}`; // Deduplicate per person per PR - if (!addedReviewPRs.has(reviewKey)) { - addedReviewPRs.add(reviewKey); - return SUMMARY_GROUP_NAMES.PRS_REVIEWED; - } - return null; // Skip duplicate reviews from same person on same PR - } - - if (type === 'comment' && item.title?.startsWith('Review comment on:')) { - // Review comments on PRs are ignored (section removed) - return null; - } - - if (type === 'comment') { - return SUMMARY_GROUP_NAMES.COMMENTS; - } - - if (type === 'commit') { - return SUMMARY_GROUP_NAMES.COMMITS; - } - - if (type === 'other') { - return SUMMARY_GROUP_NAMES.OTHER_EVENTS; - } - - if (type === 'pull_request') { - const mergedAt = item.merged_at || item.pull_request?.merged_at; - const createdInRange = isDateInRange(item.created_at, startDate, endDate); - const mergedInRange = mergedAt && isDateInRange(mergedAt, startDate, endDate); - const closedInRange = item.closed_at && isDateInRange(item.closed_at, startDate, endDate); - const updatedInRange = isDateInRange(item.updated_at, startDate, endDate); - - - - // Check if PR was merged within the timeframe - if (mergedAt && mergedInRange) { - return SUMMARY_GROUP_NAMES.PRS_MERGED; - } - // Check if PR was closed within the timeframe (and not merged) - else if (item.state === 'closed' && closedInRange && !mergedAt) { - return SUMMARY_GROUP_NAMES.PRS_CLOSED; - } - // Check if PR was created within the timeframe - else if (createdInRange) { - return SUMMARY_GROUP_NAMES.PRS_OPENED; - } - // Check if PR was updated but not created/merged/closed within the timeframe - else if (updatedInRange && !createdInRange && !mergedInRange && !closedInRange) { - return SUMMARY_GROUP_NAMES.PRS_UPDATED; - } - - // If none of the above, this PR doesn't belong in this timeframe summary - return null; - } - - // Handle issues - apply date range filtering regardless of authorship - // Ensure this is actually an issue and not a PR - if (type === 'issue' && !item.pull_request) { - const createdInRange = isDateInRange(item.created_at, startDate, endDate); - const closedInRange = item.closed_at && isDateInRange(item.closed_at, startDate, endDate); - const updatedInRange = isDateInRange(item.updated_at, startDate, endDate); - - if (item.state === 'closed' && closedInRange) { - // Issue was closed within the timeframe - return SUMMARY_GROUP_NAMES.ISSUES_CLOSED; - } else if (createdInRange && (!item.action || item.action === 'opened')) { - // Issue was created in range - either explicitly opened or no action specified (from Search API) - return SUMMARY_GROUP_NAMES.ISSUES_OPENED; - } else if (updatedInRange) { - // Issue was updated in the timeframe (labeled, assigned, etc.) but was not categorized as opened/closed in this summary - return SUMMARY_GROUP_NAMES.ISSUES_UPDATED; - } - - // If none of the above conditions are met, filter out the issue - return null; - } - - // Default fallback (shouldn't reach here for normal issues) - return null; -}; + endDate: string, +): SummaryGroupName | null => categorizeItem(item, addedReviewPRs, startDate, endDate, false); /** * Groups GitHub items into summary categories @@ -280,15 +153,13 @@ export const groupItems = ( items: GitHubItem[], startDate: string, endDate: string, - applyDateFiltering = true + applyDateFiltering = true, ): Record => { const groups = createEmptyGroups(); - const addedReviewKeys = new Set(); // Tracks person:PR combinations for review deduplication + const addedReviewKeys = new Set(); items.forEach(item => { - const groupName = applyDateFiltering - ? categorizeItem(item, addedReviewKeys, startDate, endDate) - : categorizeItemWithoutDateFiltering(item, addedReviewKeys, startDate, endDate); + const groupName = categorizeItem(item, addedReviewKeys, startDate, endDate, applyDateFiltering); if (groupName) { groups[groupName].push(item); } @@ -304,18 +175,17 @@ export const addMergedPRsFromSearchItems = ( groups: Record, searchItems: GitHubItem[], startDate: string, - endDate: string + endDate: string, ): void => { const startDateTime = new Date(startDate).getTime(); - const endDateTime = new Date(endDate).getTime() + 24 * 60 * 60 * 1000 - 1; // End of day + const endDateTime = new Date(endDate).getTime() + 24 * 60 * 60 * 1000 - 1; const existingMergedPRUrls = new Set(groups[SUMMARY_GROUP_NAMES.PRS_MERGED].map(item => item.html_url)); - + searchItems.forEach(searchItem => { const mergedAt = searchItem.merged_at || searchItem.pull_request?.merged_at; if (searchItem.pull_request && mergedAt && !existingMergedPRUrls.has(searchItem.html_url)) { - const mergeDate = new Date(mergedAt); - const mergeTime = mergeDate.getTime(); + const mergeTime = new Date(mergedAt).getTime(); if (mergeTime >= startDateTime && mergeTime <= endDateTime) { groups[SUMMARY_GROUP_NAMES.PRS_MERGED].push(searchItem); } @@ -330,7 +200,7 @@ export const addIssuesFromSearchItems = ( groups: Record, searchItems: GitHubItem[], startDate: string, - endDate: string + endDate: string, ): void => { const existingIssueUrls = new Set([ ...groups[SUMMARY_GROUP_NAMES.ISSUES_OPENED].map(item => item.html_url), @@ -339,14 +209,12 @@ export const addIssuesFromSearchItems = ( ]); searchItems.forEach(searchItem => { - // Explicitly filter out PRs to ensure they don't appear in issue sections if (!searchItem.pull_request && !existingIssueUrls.has(searchItem.html_url)) { - // Categorize the issue using the same logic as other issues - const addedReviewPRs = new Set(); // Empty set since we're only dealing with issues + const addedReviewPRs = new Set(); const groupName = categorizeItem(searchItem, addedReviewPRs, startDate, endDate); - - if (groupName && (groupName === SUMMARY_GROUP_NAMES.ISSUES_OPENED || - groupName === SUMMARY_GROUP_NAMES.ISSUES_CLOSED || + + if (groupName && (groupName === SUMMARY_GROUP_NAMES.ISSUES_OPENED || + groupName === SUMMARY_GROUP_NAMES.ISSUES_CLOSED || groupName === SUMMARY_GROUP_NAMES.ISSUES_UPDATED)) { groups[groupName].push(searchItem); } @@ -361,18 +229,10 @@ export const groupSummaryData = ( items: GitHubItem[], searchItems: GitHubItem[], startDate: string, - endDate: string + endDate: string, ): Record => { - // Group items first (apply proper date filtering for categorization) const groups = groupItems(items, startDate, endDate, true); - - // Add merged PRs from search items addMergedPRsFromSearchItems(groups, searchItems, startDate, endDate); - - // Add issues from search items addIssuesFromSearchItems(groups, searchItems, startDate, endDate); - - - return groups; -}; \ No newline at end of file +}; diff --git a/src/utils/viewFiltering.ts b/src/utils/viewFiltering.ts index f8aaa16..6a85267 100644 --- a/src/utils/viewFiltering.ts +++ b/src/utils/viewFiltering.ts @@ -127,18 +127,4 @@ export const sortItemsByUpdatedDate = (items: GitHubItem[]): GitHubItem[] => { ); }; -/** - * Parses comma-separated usernames (common pattern in IssuesAndPRsList) - */ -export const parseCommaSeparatedUsernames = (username: string): string[] => { - return username.split(',').map(u => u.trim().toLowerCase()); -}; - -/** - * Checks if an item is authored by any of the searched users (common pattern) - */ -export const isItemAuthoredBySearchedUsers = (item: GitHubItem, searchedUsernames: string[]): boolean => { - const itemAuthor = item.user.login.toLowerCase(); - const searchedUsernamesLower = searchedUsernames.map(username => username.toLowerCase()); - return searchedUsernamesLower.includes(itemAuthor); -}; \ No newline at end of file + \ No newline at end of file diff --git a/src/views/EventView.tsx b/src/views/EventView.tsx index f6ad6be..0325f93 100644 --- a/src/views/EventView.tsx +++ b/src/views/EventView.tsx @@ -1,4 +1,4 @@ -import { memo, useMemo, useState, useCallback, useEffect } from 'react'; +import { memo, useEffect } from 'react'; import { Text, Checkbox, @@ -9,6 +9,8 @@ import { GitHubItem, GitHubEvent } from '../types'; import { ResultsContainer } from '../components/ResultsContainer'; import { useCopyFeedback } from '../hooks/useCopyFeedback'; +import { useListSelection } from '../hooks/useListSelection'; +import { useDialogNavigation } from '../hooks/useDialogNavigation'; import { filterItemsByAdvancedSearch, sortItemsByUpdatedDate } from '../utils/viewFiltering'; import { copyResultsToClipboard as copyToClipboard } from '../utils/clipboard'; @@ -22,8 +24,6 @@ import { useLocalStorage } from '../hooks/useLocalStorage'; import { DismissibleBanner } from '../components/DismissibleBanner'; import { useFormContext } from '../App'; - - interface EventViewProps { items: GitHubItem[]; rawEvents?: GitHubEvent[]; @@ -33,39 +33,27 @@ const EventView = memo(function EventView({ items, rawEvents = [], }: EventViewProps) { - // Get shared search text from form context const { searchText, setSearchText } = useFormContext(); - - // Internal state for selection - const [selectedItems, setSelectedItems] = useLocalStorage>('eventView-selectedItems', new Set()); - // Pagination state const [currentPage, setCurrentPage] = useLocalStorage('eventView-currentPage', 1); const itemsPerPage = 100; - // Filter and sort items using utility functions + // Filter and sort items const filteredItems = filterItemsByAdvancedSearch(items, searchText); const sortedItems = sortItemsByUpdatedDate(filteredItems); - - // Internal selection handlers - const toggleItemSelection = useCallback((id: string | number) => { - setSelectedItems(prev => { - const newSet = new Set(prev); - if (newSet.has(id)) { - newSet.delete(id); - } else { - newSet.add(id); - } - return newSet; - }); - }, []); - const clearSelection = useCallback(() => { - setSelectedItems(new Set()); - }, []); + // Shared hooks + const { + selectedItems, toggleItemSelection, selectAllItems, clearSelection, + selectAllState, + } = useListSelection('eventView-selectedItems', sortedItems); + + const { + selectedItemForDialog, setSelectedItemForDialog, + handlePreviousItem, handleNextItem, hasPrevious, hasNext, + } = useDialogNavigation(sortedItems); - // Use copy feedback hook const { isCopied, triggerCopy } = useCopyFeedback(2000); // Reset to first page when search changes @@ -73,24 +61,13 @@ const EventView = memo(function EventView({ setCurrentPage(1); }, [searchText]); - // Calculate pagination + // Pagination const totalPages = Math.ceil(sortedItems.length / itemsPerPage); const startIndex = (currentPage - 1) * itemsPerPage; - const endIndex = startIndex + itemsPerPage; - const paginatedItems = sortedItems.slice(startIndex, endIndex); + const paginatedItems = sortedItems.slice(startIndex, startIndex + itemsPerPage); - // Handle page change - const handlePageChange = useCallback((_event: React.MouseEvent, page: number) => { - setCurrentPage(page); - }, []); - - // Select all items across all pages - const selectAllItems = useCallback(() => { - setSelectedItems(new Set(sortedItems.map((item: GitHubItem) => item.event_id || item.id))); - }, [sortedItems]); - - // Internal copy handler for content - const copyResultsToClipboard = useCallback(async (format: 'detailed' | 'compact') => { + // Copy handler + const copyResultsToClipboard = async (format: 'detailed' | 'compact') => { const selectedItemsArray = selectedItems.size > 0 ? sortedItems.filter((item: GitHubItem) => @@ -100,145 +77,60 @@ const EventView = memo(function EventView({ await copyToClipboard(selectedItemsArray, { isCompactView: format === 'compact', - onSuccess: () => { - // Trigger visual feedback via copy feedback system - triggerCopy(format); - }, - onError: (error: Error) => { - console.error('Failed to copy results:', error); - }, + onSuccess: () => triggerCopy(format), + onError: (error: Error) => console.error('Failed to copy results:', error), }); - }, [sortedItems, selectedItems, triggerCopy]); - - - - // Clipboard feedback helper - const isClipboardCopied = useCallback((itemId: string | number) => { - return isCopied(itemId); - }, [isCopied]); - - // Calculate select all checkbox state - const selectAllState = useMemo(() => { - if (sortedItems.length === 0) { - return { checked: false, indeterminate: false }; - } - - const selectedCount = sortedItems.filter(item => - selectedItems.has(item.event_id || item.id) - ).length; - - if (selectedCount === 0) { - return { checked: false, indeterminate: false }; - } else if (selectedCount === sortedItems.length) { - return { checked: true, indeterminate: false }; - } else { - return { checked: false, indeterminate: true }; - } - }, [sortedItems, selectedItems]); + }; - // Handle select all checkbox click + // Select all toggle const handleSelectAllChange = () => { - const selectedCount = sortedItems.filter((item: GitHubItem) => - selectedItems.has(item.event_id || item.id) - ).length; - - if (selectedCount === sortedItems.length) { - // All are selected, clear selection + if (selectAllState.checked) { clearSelection(); } else { - // Some or none are selected, select all selectAllItems(); } }; - // Description dialog state and handlers - const [selectedItemForDialog, setSelectedItemForDialog] = - useState(null); - - - - // Check if we have no results but should show different messages const hasRawEvents = rawEvents && rawEvents.length > 0; const hasSearchText = searchText && searchText.trim().length > 0; - // Dialog navigation handlers - const handlePreviousItem = () => { - if (!selectedItemForDialog) return; - const currentIndex = sortedItems.findIndex( - item => item.id === selectedItemForDialog.id - ); - if (currentIndex > 0) { - setSelectedItemForDialog(sortedItems[currentIndex - 1]); - } - }; - - const handleNextItem = () => { - if (!selectedItemForDialog) return; - const currentIndex = sortedItems.findIndex( - (item: GitHubItem) => item.id === selectedItemForDialog.id - ); - if (currentIndex < sortedItems.length - 1) { - setSelectedItemForDialog(sortedItems[currentIndex + 1]); - } - }; - - const getCurrentItemIndex = () => { - if (!selectedItemForDialog) return -1; - return sortedItems.findIndex((item: GitHubItem) => item.id === selectedItemForDialog.id); - }; - - // Header left content - const headerLeft = ( - <> - - - - Select All - {totalPages > 1 && ( - - (Page {currentPage} of {totalPages}) - - )} - - - - - - ); - - // Header right content - const headerRight = null; - return ( + + + + Select All + {totalPages > 1 && ( + + (Page {currentPage} of {totalPages}) + + )} + + + + + } + headerRight={null} className="timeline-view" > - {/* API Limitation Note */} Note: Events includes up to 300 events from the past 30 days. Event latency can be 30s to 6h depending on time of day. - {/* Timeline content */}
{sortedItems.length === 0 ? ( setSearchText('')} /> ) : ( - // Standard timeline view <> {paginatedItems.map((item: GitHubItem, index: number) => ( ))} - - {/* Pagination */} {totalPages > 1 && ( - + setCurrentPage(page)} showPages={{ narrow: false }} marginPageCount={2} surroundingPageCount={2} @@ -287,19 +170,16 @@ const EventView = memo(function EventView({ )}
- {/* Description Dialog */} setSelectedItemForDialog(null)} onPrevious={handlePreviousItem} onNext={handleNextItem} - hasPrevious={getCurrentItemIndex() > 0} - hasNext={getCurrentItemIndex() < sortedItems.length - 1} + hasPrevious={hasPrevious} + hasNext={hasNext} /> - -
); }); -export default EventView; \ No newline at end of file +export default EventView; diff --git a/src/views/IssuesAndPRsList.tsx b/src/views/IssuesAndPRsList.tsx index d23fb8d..1d3ea33 100644 --- a/src/views/IssuesAndPRsList.tsx +++ b/src/views/IssuesAndPRsList.tsx @@ -1,4 +1,4 @@ -import React, { memo, useState, useMemo, useCallback, useEffect } from 'react'; +import React, { memo, useMemo, useCallback, useEffect } from 'react'; import { Box, Text, @@ -18,16 +18,15 @@ import { import { GitHubItem } from '../types'; - import { useCopyFeedback } from '../hooks/useCopyFeedback'; +import { useListSelection } from '../hooks/useListSelection'; +import { useDialogNavigation } from '../hooks/useDialogNavigation'; import { useFormContext } from '../App'; import { copyResultsToClipboard as copyToClipboard } from '../utils/clipboard'; -import { sortItemsByUpdatedDate } from '../utils/viewFiltering'; -import { filterItemsByAdvancedSearch } from '../utils/viewFiltering'; +import { sortItemsByUpdatedDate, filterItemsByAdvancedSearch } from '../utils/viewFiltering'; import { ResultsContainer } from '../components/ResultsContainer'; - import DescriptionDialog from '../components/DescriptionDialog'; import BulkCopyButtons from '../components/BulkCopyButtons'; import EmptyState from '../components/EmptyState'; @@ -35,65 +34,35 @@ import './EventView.css'; import ItemRow from '../components/ItemRow'; import { useLocalStorage } from '../hooks/useLocalStorage'; -// Props interface -interface IssuesAndPRsListProps { - results: GitHubItem[]; - buttonStyles: React.CSSProperties; -} - // Custom title component for the description dialog const DialogTitle = ({ item }: { item: GitHubItem }) => ( - + {item.pull_request ? ( item.pull_request.merged_at ? ( - - - + ) : item.state === 'closed' ? ( - - - + ) : ( - {item.draft || item.pull_request.draft ? ( - - ) : ( - - )} + {item.draft || item.pull_request.draft ? : } ) ) : ( - + )} - - {item.title} - + {item.title} ); const IssuesAndPRsList = memo(function IssuesAndPRsList({ results, - buttonStyles, -}: IssuesAndPRsListProps) { - // Get shared search text from form context +}: { + results: GitHubItem[]; +}) { const { searchText, setSearchText } = useFormContext(); - // Internal state management (previously from context) - - // Internal state for selection and collapsed sections - const [selectedItems, setSelectedItems] = useLocalStorage>('issuesAndPRs-selectedItems', new Set()); const [collapsedSections, setCollapsedSections] = useLocalStorage>('issuesAndPRs-collapsedSections', new Set()); // Per-section pagination state @@ -113,240 +82,107 @@ const IssuesAndPRsList = memo(function IssuesAndPRsList({ setSectionPages({}); }, [searchText]); - // Use copy feedback hook const { isCopied, triggerCopy } = useCopyFeedback(2000); - // Apply search text filtering to results + // Filter and group const filteredResults = useMemo(() => { return filterItemsByAdvancedSearch(results, searchText); }, [results, searchText]); - // Define helper variables for empty state logic (consistent with other views) const hasRawData = results && results.length > 0; const hasSearchText = searchText && searchText.trim().length > 0; - // Group items into sections const groupedItems = useMemo(() => { - const groups: { - 'PRs': GitHubItem[]; - 'Issues': GitHubItem[]; - } = { - 'PRs': [], - 'Issues': [], - }; - + const groups: { 'PRs': GitHubItem[]; 'Issues': GitHubItem[] } = { 'PRs': [], 'Issues': [] }; filteredResults.forEach(item => { if (item.pull_request) { - // All pull requests go to PRs section groups['PRs'].push(item); } else { - // All issues go to Issues section regardless of authorship groups['Issues'].push(item); } }); - - // Sort each group by updated date (newest first) Object.keys(groups).forEach(key => { groups[key as keyof typeof groups] = sortItemsByUpdatedDate(groups[key as keyof typeof groups]); }); - return groups; }, [filteredResults]); - // Selection handlers - const toggleItemSelection = useCallback((id: string | number) => { - setSelectedItems(prev => { - const newSet = new Set(prev); - if (newSet.has(id)) { - newSet.delete(id); - } else { - newSet.add(id); - } - return newSet; - }); - }, []); - - const selectAllItems = useCallback(() => { - const allDisplayedItems = Object.entries(groupedItems) + // Build flat list of items from expanded sections for selection + const allDisplayedItems = useMemo(() => { + return Object.entries(groupedItems) .filter(([groupName]) => !collapsedSections.has(groupName)) .flatMap(([, items]) => items); - setSelectedItems(new Set(allDisplayedItems.map((item: GitHubItem) => item.event_id || item.id))); }, [groupedItems, collapsedSections]); - const clearSelection = useCallback(() => { - setSelectedItems(new Set()); - }, []); + // Shared hooks + const { + selectedItems, toggleItemSelection, selectAllItems, clearSelection, + bulkSelectItems, selectAllState, + } = useListSelection('issuesAndPRs-selectedItems', allDisplayedItems); - const bulkSelectItems = useCallback((itemIds: (string | number)[], shouldSelect: boolean) => { - setSelectedItems(prev => { - const newSet = new Set(prev); - if (shouldSelect) { - itemIds.forEach(id => newSet.add(id)); - } else { - itemIds.forEach(id => newSet.delete(id)); - } - return newSet; - }); - }, []); + const { + selectedItemForDialog, setSelectedItemForDialog, + handlePreviousItem, handleNextItem, hasPrevious, hasNext, + } = useDialogNavigation(allDisplayedItems); - // Toggle section collapse state and clear selections when collapsing + // Toggle section collapse and clear selections on collapse const toggleSectionCollapse = useCallback((sectionName: string) => { setCollapsedSections(prev => { const newSet = new Set(prev); - const isCurrentlyCollapsed = newSet.has(sectionName); - - if (isCurrentlyCollapsed) { - // Expanding the section + if (newSet.has(sectionName)) { newSet.delete(sectionName); } else { - // Collapsing the section - clear any selected items in this section newSet.add(sectionName); - - // Find all items in this section and remove them from selection const sectionItems = groupedItems[sectionName as keyof typeof groupedItems] || []; if (sectionItems.length > 0) { - setSelectedItems(prevSelected => { - const newSelected = new Set(prevSelected); - sectionItems.forEach(item => { - newSelected.delete(item.event_id || item.id); - }); - return newSelected; - }); + const idsToRemove = sectionItems.map(item => item.event_id || item.id); + bulkSelectItems(idsToRemove, false); } } return newSet; }); - }, [groupedItems, setSelectedItems]); + }, [groupedItems, bulkSelectItems]); - // Copy results to clipboard for content + // Copy handler const copyResultsToClipboard = useCallback(async (format: 'detailed' | 'compact') => { - // Convert to the format expected by clipboard utility const groupedData = Object.entries(groupedItems) .filter(([, items]) => items.length > 0) - .map(([groupName, items]) => ({ - groupName, - items, - })); + .map(([groupName, items]) => ({ groupName, items })); - // Use the enhanced clipboard utility with grouped data - // If items are selected, filter the grouped data to only include selected items let finalGroupedData = groupedData; if (selectedItems.size > 0) { - finalGroupedData = groupedData.map(group => ({ - ...group, - items: group.items.filter(item => - selectedItems.has(item.event_id || item.id) - ) - })).filter(group => group.items.length > 0); + finalGroupedData = groupedData + .map(group => ({ + ...group, + items: group.items.filter(item => selectedItems.has(item.event_id || item.id)), + })) + .filter(group => group.items.length > 0); } - // Get all items for the clipboard (either selected or all) const allItems = Object.values(groupedItems).flat(); const selectedItemsArray = selectedItems.size > 0 - ? allItems.filter(item => - selectedItems.has(item.event_id || item.id) - ) + ? allItems.filter(item => selectedItems.has(item.event_id || item.id)) : allItems; await copyToClipboard(selectedItemsArray, { isCompactView: format === 'compact', isGroupedView: true, groupedData: finalGroupedData, - onSuccess: () => { - triggerCopy(format); - }, - onError: (error: Error) => { - console.error('Failed to copy grouped results:', error); - }, + onSuccess: () => triggerCopy(format), + onError: (error: Error) => console.error('Failed to copy grouped results:', error), }); }, [groupedItems, selectedItems, triggerCopy]); - // Clipboard feedback helper - const isClipboardCopied = useCallback((itemId: string | number) => { - return isCopied(itemId); - }, [isCopied]); - - // Calculate select all checkbox state (only consider expanded sections) - const selectAllState = useMemo(() => { - const allDisplayedItems = Object.entries(groupedItems) - .filter(([groupName]) => !collapsedSections.has(groupName)) - .flatMap(([, items]) => items); - - if (allDisplayedItems.length === 0) { - return { checked: false, indeterminate: false }; - } - - const selectedCount = allDisplayedItems.filter(item => - selectedItems.has(item.event_id || item.id) - ).length; - - if (selectedCount === 0) { - return { checked: false, indeterminate: false }; - } else if (selectedCount === allDisplayedItems.length) { - return { checked: true, indeterminate: false }; - } else { - return { checked: false, indeterminate: true }; - } - }, [groupedItems, selectedItems, collapsedSections]); - - // Handle select all checkbox click (only consider expanded sections) + // Select all toggle const handleSelectAllChange = () => { - const allDisplayedItems = Object.entries(groupedItems) - .filter(([groupName]) => !collapsedSections.has(groupName)) - .flatMap(([, items]) => items); - - const selectedCount = allDisplayedItems.filter(item => - selectedItems.has(item.event_id || item.id) - ).length; - - if (selectedCount === allDisplayedItems.length) { - // All are selected, clear selection + if (selectAllState.checked) { clearSelection(); } else { - // Some or none are selected, select all selectAllItems(); } }; - // Dialog state - const [selectedItemForDialog, setSelectedItemForDialog] = - useState(null); - - - // Dialog navigation handlers - const handlePreviousItem = () => { - if (!selectedItemForDialog) return; - const allItems = Object.values(groupedItems).flat(); - const currentIndex = allItems.findIndex( - item => item.id === selectedItemForDialog.id - ); - if (currentIndex > 0) { - setSelectedItemForDialog(allItems[currentIndex - 1]); - } - }; - - const handleNextItem = () => { - if (!selectedItemForDialog) return; - const allItems = Object.values(groupedItems).flat(); - const currentIndex = allItems.findIndex( - item => item.id === selectedItemForDialog.id - ); - if (currentIndex < allItems.length - 1) { - setSelectedItemForDialog(allItems[currentIndex + 1]); - } - }; - - const getCurrentItemIndex = () => { - if (!selectedItemForDialog) return -1; - const allItems = Object.values(groupedItems).flat(); - return allItems.findIndex(item => item.id === selectedItemForDialog.id); - }; - - const allDisplayedItems = Object.entries(groupedItems) - .filter(([groupName]) => !collapsedSections.has(groupName)) - .flatMap(([, items]) => items); - return ( - + Select All @@ -383,138 +212,117 @@ const IssuesAndPRsList = memo(function IssuesAndPRsList({ headerRight={null} >
- {/* Results List */} - {(() => { - if (allDisplayedItems.length === 0) { - return ( - setSearchText('')} - /> - ); - } - - return ( -
- {Object.entries(groupedItems).map(([groupName, groupItems]) => { - if (groupItems.length === 0) return null; - - return ( -
-
- - - { - const sectionItemIds = groupItems.map(item => item.event_id || item.id); - return sectionItemIds.length > 0 && sectionItemIds.every(id => selectedItems.has(id)); - })()} - indeterminate={(() => { - const sectionItemIds = groupItems.map(item => item.event_id || item.id); - const selectedCount = sectionItemIds.filter(id => selectedItems.has(id)).length; - return selectedCount > 0 && selectedCount < sectionItemIds.length; - })()} - onChange={() => { - // Don't allow selection if section is collapsed - if (collapsedSections.has(groupName)) return; - - const sectionItemIds = groupItems.map(item => item.event_id || item.id); - const selectedCount = sectionItemIds.filter(id => selectedItems.has(id)).length; - const allSelected = selectedCount === sectionItemIds.length; - bulkSelectItems(sectionItemIds, !allSelected); - }} - sx={{ flexShrink: 0 }} - aria-label={`Select all items in ${groupName} section`} - disabled={collapsedSections.has(groupName)} - /> - - {groupName} ({groupItems.length}) - - - + sx={{ flexShrink: 0 }} + aria-label={`Select all items in ${groupName} section`} + disabled={collapsedSections.has(groupName)} + /> + + {groupName} ({groupItems.length}) + -
- {!collapsedSections.has(groupName) && (() => { - const sectionPage = getSectionPage(groupName); - const sectionTotalPages = Math.ceil(groupItems.length / itemsPerPage); - const startIndex = (sectionPage - 1) * itemsPerPage; - const paginatedGroupItems = groupItems.slice(startIndex, startIndex + itemsPerPage); - - return ( -
- {paginatedGroupItems.map((item: GitHubItem) => ( -
- -
- ))} - {sectionTotalPages > 1 && ( - - setSectionPage(groupName, page)} - showPages={{ narrow: false }} - marginPageCount={2} - surroundingPageCount={2} - /> - - )} -
- ); - })()} + +
- ); - })} -
- ); - })()} + {!collapsedSections.has(groupName) && (() => { + const sectionPage = getSectionPage(groupName); + const sectionTotalPages = Math.ceil(groupItems.length / itemsPerPage); + const startIndex = (sectionPage - 1) * itemsPerPage; + const paginatedGroupItems = groupItems.slice(startIndex, startIndex + itemsPerPage); + return ( +
+ {paginatedGroupItems.map((item: GitHubItem) => ( +
+ +
+ ))} + {sectionTotalPages > 1 && ( + + setSectionPage(groupName, page)} + showPages={{ narrow: false }} + marginPageCount={2} + surroundingPageCount={2} + /> + + )} +
+ ); + })()} +
+ ); + })} + + )} - {/* Description Dialog */} setSelectedItemForDialog(null)} onPrevious={handlePreviousItem} onNext={handleNextItem} - hasPrevious={getCurrentItemIndex() > 0} - hasNext={getCurrentItemIndex() < allDisplayedItems.length - 1} + hasPrevious={hasPrevious} + hasNext={hasNext} title={selectedItemForDialog ? : undefined} maxHeight="85vh" /> - -
); }); diff --git a/src/views/Summary.tsx b/src/views/Summary.tsx index c83a2e9..319a925 100644 --- a/src/views/Summary.tsx +++ b/src/views/Summary.tsx @@ -1,4 +1,4 @@ -import { memo, useMemo, useState, useCallback } from 'react'; +import { memo, useMemo, useCallback } from 'react'; import { Text, Button, @@ -16,6 +16,8 @@ import { GitHubItem, GitHubEvent } from '../types'; import { ResultsContainer } from '../components/ResultsContainer'; import { copyResultsToClipboard as copyToClipboard } from '../utils/clipboard'; import { useCopyFeedback } from '../hooks/useCopyFeedback'; +import { useListSelection } from '../hooks/useListSelection'; +import { useDialogNavigation } from '../hooks/useDialogNavigation'; import { filterItemsByAdvancedSearch, sortItemsByUpdatedDate } from '../utils/viewFiltering'; import DescriptionDialog from '../components/DescriptionDialog'; @@ -26,15 +28,13 @@ import './Summary.css'; import { useFormContext } from '../App'; import { useLocalStorage } from '../hooks/useLocalStorage'; import { groupSummaryData, getEventType } from '../utils/summaryGrouping'; -import { - formatGroupedDataForClipboard, - getAllDisplayedItems, - hasAnyItems, - getGroupSelectState +import { + formatGroupedDataForClipboard, + getAllDisplayedItems, + getGroupSelectState, } from '../utils/summaryHelpers'; import { DismissibleBanner } from '../components/DismissibleBanner'; - interface SummaryProps { items: GitHubItem[]; rawEvents?: GitHubEvent[]; @@ -46,268 +46,128 @@ const SummaryView = memo(function SummaryView({ rawEvents = [], indexedDBSearchItems = [], }: SummaryProps) { - // Get form settings from form context const { startDate, endDate, searchText, setSearchText } = useFormContext(); - - // Internal state for selection and collapsed sections - const [selectedItems, setSelectedItems] = useLocalStorage>('summary-selectedItems', new Set()); + const [collapsedSections, setCollapsedSections] = useLocalStorage>('summary-collapsedSections', new Set()); - - // Filter and sort items using utility functions + + // Filter and sort items const filteredItems = filterItemsByAdvancedSearch(items, searchText); const sortedItems = sortItemsByUpdatedDate(filteredItems); - - // Internal selection handlers - const toggleItemSelection = useCallback((id: string | number) => { - setSelectedItems(prev => { - const newSet = new Set(prev); - if (newSet.has(id)) { - newSet.delete(id); - } else { - newSet.add(id); - } - return newSet; - }); - }, []); - - const clearSelection = useCallback(() => { - setSelectedItems(new Set()); - }, []); - - const bulkSelectItems = useCallback((itemIds: (string | number)[], shouldSelect: boolean) => { - setSelectedItems(prev => { - const newSet = new Set(prev); - if (shouldSelect) { - itemIds.forEach(id => newSet.add(id)); - } else { - itemIds.forEach(id => newSet.delete(id)); - } - return newSet; - }); - }, []); - - // Use copy feedback hook - const { isCopied, triggerCopy } = useCopyFeedback(2000); - // Filter IndexedDB search items with the same search criteria + // Filtered search items for summary grouping const filteredIndexedDBSearchItems = useMemo(() => { return filterItemsByAdvancedSearch(indexedDBSearchItems, searchText); }, [indexedDBSearchItems, searchText]); - // Grouping logic for summary view using extracted utility functions + // Group items for summary view const actionGroups = useMemo(() => { - return groupSummaryData( - sortedItems, - filteredIndexedDBSearchItems, - startDate, - endDate - ); + return groupSummaryData(sortedItems, filteredIndexedDBSearchItems, startDate, endDate); }, [sortedItems, filteredIndexedDBSearchItems, startDate, endDate]); - // Toggle section collapse state and clear selections when collapsing + // Build flat list of items from expanded sections for selection + const allDisplayedItems = useMemo(() => { + return Object.entries(actionGroups) + .filter(([groupName]) => !collapsedSections.has(groupName)) + .flatMap(([, items]) => items); + }, [actionGroups, collapsedSections]); + + // Shared hooks + const { + selectedItems, toggleItemSelection, selectAllItems, clearSelection, + bulkSelectItems, selectAllState, + } = useListSelection('summary-selectedItems', allDisplayedItems); + + const { + selectedItemForDialog, setSelectedItemForDialog, + handlePreviousItem, handleNextItem, hasPrevious, hasNext, + } = useDialogNavigation(sortedItems); + + const { isCopied, triggerCopy } = useCopyFeedback(2000); + + // Toggle section collapse and clear selections on collapse const toggleSectionCollapse = useCallback((sectionName: string) => { setCollapsedSections(prev => { const newSet = new Set(prev); - const isCurrentlyCollapsed = newSet.has(sectionName); - - if (isCurrentlyCollapsed) { - // Expanding the section + if (newSet.has(sectionName)) { newSet.delete(sectionName); } else { - // Collapsing the section - clear any selected items in this section newSet.add(sectionName); - - // Find all items in this section and remove them from selection const sectionItems = actionGroups[sectionName as keyof typeof actionGroups] || []; if (sectionItems.length > 0) { - setSelectedItems(prevSelected => { - const newSelected = new Set(prevSelected); - sectionItems.forEach(item => { - newSelected.delete(item.event_id || item.id); - }); - return newSelected; - }); + const idsToRemove = sectionItems.map(item => item.event_id || item.id); + bulkSelectItems(idsToRemove, false); } } return newSet; }); - }, [actionGroups, setSelectedItems]); + }, [actionGroups, bulkSelectItems]); - // Select all items that are actually displayed in the view (only from expanded sections) - const selectAllItems = useCallback(() => { - const allDisplayedItems = Object.entries(actionGroups) - .filter(([groupName]) => !collapsedSections.has(groupName)) - .flatMap(([, items]) => items); - setSelectedItems(new Set(allDisplayedItems.map(item => item.event_id || item.id))); - }, [actionGroups, collapsedSections]); - - // Internal copy handler for content + // Copy handler const copyResultsToClipboard = useCallback(async (format: 'detailed' | 'compact') => { const groupedData = formatGroupedDataForClipboard(actionGroups, selectedItems); - - // Get all items for the clipboard (either selected or all) const allItems = getAllDisplayedItems(actionGroups); const selectedItemsArray = selectedItems.size > 0 - ? allItems.filter(item => - selectedItems.has(item.event_id || item.id) - ) + ? allItems.filter(item => selectedItems.has(item.event_id || item.id)) : allItems; await copyToClipboard(selectedItemsArray, { isCompactView: format === 'compact', isGroupedView: true, groupedData, - onSuccess: () => { - // Trigger visual feedback via copy feedback system - triggerCopy(format); - }, - onError: (error: Error) => { - console.error('Failed to copy grouped results:', error); - }, + onSuccess: () => triggerCopy(format), + onError: (error: Error) => console.error('Failed to copy grouped results:', error), }); }, [actionGroups, selectedItems, triggerCopy]); - - - // Clipboard feedback helper - const isClipboardCopied = useCallback((itemId: string | number) => { - return isCopied(itemId); - }, [isCopied]); - - // Calculate select all checkbox state (only consider expanded sections) - const selectAllState = useMemo(() => { - const allDisplayedItems = Object.entries(actionGroups) - .filter(([groupName]) => !collapsedSections.has(groupName)) - .flatMap(([, items]) => items); - - if (allDisplayedItems.length === 0) { - return { checked: false, indeterminate: false }; - } - - const selectedCount = allDisplayedItems.filter(item => - selectedItems.has(item.event_id || item.id) - ).length; - - if (selectedCount === 0) { - return { checked: false, indeterminate: false }; - } else if (selectedCount === allDisplayedItems.length) { - return { checked: true, indeterminate: false }; - } else { - return { checked: false, indeterminate: true }; - } - }, [actionGroups, selectedItems, collapsedSections]); - - // Handle select all checkbox click (only consider expanded sections) + // Select all toggle const handleSelectAllChange = () => { - const allDisplayedItems = Object.entries(actionGroups) - .filter(([groupName]) => !collapsedSections.has(groupName)) - .flatMap(([, items]) => items); - - const selectedCount = allDisplayedItems.filter(item => - selectedItems.has(item.event_id || item.id) - ).length; - - if (selectedCount === allDisplayedItems.length) { - // All are selected, clear selection - clearSelection?.(); + if (selectAllState.checked) { + clearSelection(); } else { - // Some or none are selected, select all selectAllItems(); } }; - // Description dialog state and handlers - const [selectedItemForDialog, setSelectedItemForDialog] = - useState(null); - - - - // Single item clipboard copy handler - - - // Dialog navigation handlers - const handlePreviousItem = () => { - if (!selectedItemForDialog) return; - const currentIndex = sortedItems.findIndex( - item => item.id === selectedItemForDialog.id - ); - if (currentIndex > 0) { - setSelectedItemForDialog(sortedItems[currentIndex - 1]); - } - }; - - const handleNextItem = () => { - if (!selectedItemForDialog) return; - const currentIndex = sortedItems.findIndex( - item => item.id === selectedItemForDialog.id - ); - if (currentIndex < sortedItems.length - 1) { - setSelectedItemForDialog(sortedItems[currentIndex + 1]); - } - }; - - const getCurrentItemIndex = () => { - if (!selectedItemForDialog) return -1; - return sortedItems.findIndex(item => item.id === selectedItemForDialog.id); - }; - - - - // Check if we have no results but should show different messages const hasRawEvents = rawEvents && rawEvents.length > 0; const hasSearchText = searchText && searchText.trim().length > 0; - - // Header left content - const headerLeft = ( - <> - - - - Select All - - - - - - ); - - // Header right content - const headerRight = null; + const hasItems = Object.values(actionGroups).some(items => items.length > 0); return ( + + + + Select All + + + + + } + headerRight={null} className="timeline-view" > - {/* API Limitation Note */} - Note: This view merges the last 100 GitHub issues/PRs and 300 public GitHub Events per user. + Note: This view merges the last 100 GitHub issues/PRs and 300 public GitHub Events per user. Event latency can be 30s to 6h depending on time of day. - {/* Timeline content */}
- {!hasAnyItems(actionGroups) ? ( + {!hasItems ? ( setSearchText('')} /> ) : ( - // Grouped view - organize events by individual issues/PRs and by type Object.entries(actionGroups).map(([groupName, groupItems]) => { if (groupItems.length === 0) return null; // Group items by URL (for reviews, include user to allow multiple reviewers per PR) @@ -325,19 +184,16 @@ const SummaryView = memo(function SummaryView({ if (getEventType(item) === 'comment') { groupingKey = groupingKey.split('#')[0]; } - - // For reviews, include user in grouping key to allow multiple reviewers per PR const isReview = (item.title && item.title.startsWith('Review on:')) || item.originalEventType === 'PullRequestReviewEvent'; if (isReview) { groupingKey = `${item.user.login}:${groupingKey}`; } - if (!urlGroups[groupingKey]) { urlGroups[groupingKey] = []; } urlGroups[groupingKey].push(item); }); - // Render one ItemRow per group, showing groupCount + return (
@@ -346,20 +202,15 @@ const SummaryView = memo(function SummaryView({ { - // Don't allow selection if section is collapsed if (collapsedSections.has(groupName)) return; - const sectionItemIds = Object.values(urlGroups).map(items => { const mostRecent = items.reduce((latest, current) => - new Date(current.updated_at) > new Date(latest.updated_at) - ? current - : latest + new Date(current.updated_at) > new Date(latest.updated_at) ? current : latest ); return mostRecent.event_id || mostRecent.id; }); const selectedCount = sectionItemIds.filter(id => selectedItems.has(id)).length; - const allSelected = selectedCount === sectionItemIds.length; - bulkSelectItems(sectionItemIds, !allSelected); + bulkSelectItems(sectionItemIds, selectedCount !== sectionItemIds.length); }} sx={{ flexShrink: 0 }} aria-label={`Select all events in ${groupName} section`} @@ -369,20 +220,14 @@ const SummaryView = memo(function SummaryView({ {groupName} {(() => { - // Calculate total count (number of URL groups = number of displayed items) const totalCount = Object.keys(urlGroups).length; - - // Calculate selected count const sectionItemIds = Object.values(urlGroups).map(items => { const mostRecent = items.reduce((latest, current) => - new Date(current.updated_at) > new Date(latest.updated_at) - ? current - : latest + new Date(current.updated_at) > new Date(latest.updated_at) ? current : latest ); return mostRecent.event_id || mostRecent.id; }); const selectedCount = sectionItemIds.filter(id => selectedItems.has(id)).length; - return ( 0 ? `${selectedCount} / ${totalCount}` : `${totalCount}`} @@ -397,11 +242,11 @@ const SummaryView = memo(function SummaryView({ size="small" onClick={() => toggleSectionCollapse(groupName)} className="timeline-section-collapse-button" - sx={{ + sx={{ fontSize: '0.75rem', color: 'fg.muted', flexShrink: 0, - '&:hover': { color: 'fg.default' } + '&:hover': { color: 'fg.default' }, }} aria-label={`${collapsedSections.has(groupName) ? 'Show' : 'Hide'} ${groupName} section`} > @@ -412,11 +257,8 @@ const SummaryView = memo(function SummaryView({ {!collapsedSections.has(groupName) && (
{Object.entries(urlGroups).map(([url, items]) => { - // Show the most recent item in the group const mostRecent = items.reduce((latest, current) => - new Date(current.updated_at) > new Date(latest.updated_at) - ? current - : latest + new Date(current.updated_at) > new Date(latest.updated_at) ? current : latest ); return (
@@ -439,17 +281,14 @@ const SummaryView = memo(function SummaryView({ )}
- {/* Description Dialog */} setSelectedItemForDialog(null)} onPrevious={handlePreviousItem} onNext={handleNextItem} - hasPrevious={getCurrentItemIndex() > 0} - hasNext={getCurrentItemIndex() < sortedItems.length - 1} + hasPrevious={hasPrevious} + hasNext={hasNext} /> - - ); }); diff --git a/src/views/__tests__/IssuesAndPRsList.test.tsx b/src/views/__tests__/IssuesAndPRsList.test.tsx index 38293d3..87d02c0 100644 --- a/src/views/__tests__/IssuesAndPRsList.test.tsx +++ b/src/views/__tests__/IssuesAndPRsList.test.tsx @@ -45,7 +45,7 @@ const generateItems = (count: number, isPR: boolean, startId = 1): GitHubItem[] const renderComponent = (results: GitHubItem[]) => render( - + ); From 79ef7eed77aaebef885c80792c44a3b367cec994 Mon Sep 17 00:00:00 2001 From: Valentin Date: Thu, 19 Feb 2026 11:16:50 +0100 Subject: [PATCH 3/3] Address review feedback: share getItemId, remove IIFEs, delete dead code - Export getItemId from types.ts, replace ~20 inline event_id||id patterns - Extract SectionContent component and helpers (getMostRecent, groupItemsByUrl) to remove IIFEs from JSX and avoid redundant per-section computation - Delete CODE_REVIEW.md, dead summaryHelpers exports, compat wrapper - Fix missing useCallback/useEffect deps, remove headerRight={null} ceremony - Fix isTestEnvironment to guard process access and not require window - Memoize currentIndex in useDialogNavigation - Wrap copyResultsToClipboard in useCallback in EventView Co-Authored-By: Claude Opus 4.6 --- CODE_REVIEW.md | 235 ------------------ src/components/CopyToClipboardButton.tsx | 6 +- src/components/ItemRow.tsx | 6 +- src/hooks/useDialogNavigation.ts | 11 +- src/hooks/useListSelection.ts | 26 +- src/types.ts | 3 + src/utils/__tests__/summaryGrouping.test.ts | 15 +- src/utils/__tests__/summaryHelpers.test.ts | 72 +----- src/utils/environment.ts | 9 +- src/utils/summaryGrouping.ts | 8 - src/utils/summaryHelpers.ts | 35 +-- src/views/EventView.tsx | 15 +- src/views/IssuesAndPRsList.tsx | 128 ++++++---- src/views/Summary.tsx | 100 ++++---- src/views/__tests__/IssuesAndPRsList.test.tsx | 2 +- 15 files changed, 182 insertions(+), 489 deletions(-) delete mode 100644 CODE_REVIEW.md diff --git a/CODE_REVIEW.md b/CODE_REVIEW.md deleted file mode 100644 index 1a8072c..0000000 --- a/CODE_REVIEW.md +++ /dev/null @@ -1,235 +0,0 @@ -# Code Review: GitVegas — Readability, Simplicity, Redundancy - -Reviewed as a principal developer focusing on readability, simplicity, and eliminating redundancy. - ---- - -## CRITICAL: Duplicated Logic Across Views - -The most significant problem in this codebase is that **IssuesAndPRsList**, **SummaryView**, and **EventView** are ~70% identical. Each independently implements: - -- Selection state (`toggleItemSelection`, `clearSelection`, `bulkSelectItems`, `selectAllItems`) -- Select-all checkbox state computation (`selectAllState` useMemo) -- `handleSelectAllChange` handler -- `copyResultsToClipboard` with nearly identical clipboard formatting -- `isClipboardCopied` wrapper (a trivial pass-through to `isCopied`) -- Dialog state + navigation (`selectedItemForDialog`, `handlePreviousItem`, `handleNextItem`, `getCurrentItemIndex`) -- Empty state logic (`hasRawData`/`hasRawEvents`, `hasSearchText`) - -**Files**: `src/views/IssuesAndPRsList.tsx:86-520`, `src/views/Summary.tsx:44-457`, `src/views/EventView.tsx:32-305` - -**Recommendation**: Extract a `useListSelection` hook and a `useDialogNavigation` hook. These three views should share the behavior, not copy-paste it. Rough sketch: - -```ts -// useListSelection(items: GitHubItem[]) -// returns: selectedItems, toggleItem, selectAll, clearSelection, bulkSelect, selectAllState -``` - -```ts -// useDialogNavigation(items: GitHubItem[]) -// returns: selectedItem, setSelectedItem, handlePrev, handleNext, hasPrev, hasNext -``` - ---- - -## HIGH: Redundant `isClipboardCopied` Wrappers - -All three views define: - -```ts -const isClipboardCopied = useCallback((itemId: string | number) => { - return isCopied(itemId); -}, [isCopied]); -``` - -**Files**: `IssuesAndPRsList.tsx:266-268`, `Summary.tsx:174-176`, `EventView.tsx:116-118` - -This wraps `isCopied` in `useCallback` for no benefit -- `isCopied` is already a stable callback from `useCopyFeedback`. Just pass `isCopied` directly. - ---- - -## HIGH: Duplicated Utility Functions - -### `parseUsernames` / `parseCommaSeparatedUsernames` - -- `src/utils/summaryGrouping.ts:14-16` defines `parseUsernames` -- `src/utils/viewFiltering.ts:133-135` defines `parseCommaSeparatedUsernames` - -These are identical functions. Keep one, delete the other. - -### `isAuthoredBySearchedUser` / `isItemAuthoredBySearchedUsers` - -- `src/utils/summaryGrouping.ts:21-24` defines `isAuthoredBySearchedUser` -- `src/utils/viewFiltering.ts:140-144` defines `isItemAuthoredBySearchedUsers` - -Same function, different names. The `viewFiltering` version unnecessarily re-lowercases the array on every call (should do it once outside). Keep one. - -### `categorizeItem` / `categorizeItemWithoutDateFiltering` - -- `src/utils/summaryGrouping.ts:109-180` vs `src/utils/summaryGrouping.ts:185-274` - -These two functions share ~80% of their logic (review dedup, comment handling, commit, other, PR categorization). The only difference is whether `updatedInRange` is checked. They should be a single function with a parameter, not two separate 90-line functions. - ---- - -## HIGH: `FormContextType` Defined Twice - -- `src/App.tsx:34-59` defines a local `FormContextType` -- `src/types.ts:184-206` exports a nearly identical `FormContextType` - -The App.tsx version has extra fields (`searchText`, `setSearchText`, `searchItemsCount`, `eventsCount`, `rawEventsCount`). This is confusing -- one file exports a type that is never used for the actual context, while the actual context type is hidden in App.tsx. Consolidate into `types.ts`. - ---- - -## HIGH: ItemRow Desktop/Mobile Duplication - -`src/components/ItemRow.tsx` renders the **entire component twice**: once for desktop (lines 67-252) and once for mobile (lines 255-457). The icon logic, avatar logic, title/link rendering, and action buttons are fully duplicated using CSS `display: none` toggling. - -This doubles the DOM size for every row and makes maintenance error-prone (fix a bug in one layout, forget the other). - -**Recommendation**: Use a single layout with responsive CSS, or extract shared sub-components (`StatusIcon`, `AvatarGroup`, `TitleLink`) used by both layouts. - ---- - -## MEDIUM: Dead/Unused Code - -### `handleUsernameBlur` is empty - -`src/hooks/useGitHubFormState.ts:230-233`: -```ts -const handleUsernameBlur = useCallback(async () => { - // This can be used for additional validation if needed -}, []); -``` -An empty async callback that is threaded through the context and called in `SearchForm.tsx`. Remove it or implement it. - -### `showUser` prop accepted but unused - -`src/components/ItemRow.tsx:28` declares `showUser?: boolean` but it's never referenced in the component body. It's always passed as `showUser={true}` by callers. Dead prop. - -### Unreachable code in `useGitHubDataFetching` - -`src/hooks/useGitHubDataFetching.ts:113-115`: -```ts -throw error; -// Continue with what we have so far -hasMorePages = false; -``` -Lines 114-115 are unreachable after `throw`. The comment suggests the intent was to **not** throw. - -### `buttonStyles` export from App.tsx - -`src/App.tsx:73-80` exports a `buttonStyles` object that is only used by `IssuesAndPRsList`. It's passed as a prop through the component tree. This should live closer to where it's used, or in a shared styles file. An App-level export of style constants is odd. - ---- - -## MEDIUM: Redundant Length Check in `utils.ts` - -`src/utils.ts:82-91`: -```ts -if (trimmed.length === 0) { - return { isValid: false, error: 'Username cannot be empty' }; -} -if (trimmed.length < 1) { - return { isValid: false, error: 'Username must be at least 1 character long' }; -} -``` - -`length === 0` and `length < 1` are the same condition. The second branch is unreachable. - ---- - -## MEDIUM: Overly Complex Avatar URL Memo - -`src/App.tsx:157-164`: -```ts -const avatarUrls = useMemo(() => { - if (cachedAvatarUrls && cachedAvatarUrls.length > 0) { - return cachedAvatarUrls; - } - return []; -}, [cachedAvatarUrls]); -``` - -This memo does nothing useful. `cachedAvatarUrls` is already a memoized value from `useGitHubFormState`. At most this is `cachedAvatarUrls ?? []`, and the hook already returns `[]` as default. Remove the memo entirely. - ---- - -## MEDIUM: `as unknown as` Type Assertions - -The codebase has multiple `as unknown as` casts that indicate a type mismatch that should be fixed at the source: - -- `App.tsx:513-515`: `indexedDBSearchItems as unknown as GitHubItem[]` -- `useGitHubDataFetching.ts:292`: `sortedSearchItems as unknown as GitHubEvent[]` -- `useGitHubDataProcessing.ts:45`: `indexedDBSearchItems as unknown as GitHubItem[]` - -The `useIndexedDBStorage` hook returns `GitHubEvent[]` but search items are `GitHubItem[]`. Rather than casting everywhere, make the hook generic or create a separate hook for search items. - ---- - -## MEDIUM: `caseInsensitiveCompare` Defined Inside Component - -`src/App.tsx:167-168` defines a comparator function inside the `App` component body (not in a `useCallback` or `useMemo`). It gets recreated on every render. Since it's a pure function with no dependencies, move it outside the component. - ---- - -## LOW: Inconsistent Comment Artifacts - -Several files have blank lines where code was removed, leftover `// removed` comments, or empty blocks: - -- `Summary.tsx:171-172`: empty lines -- `Summary.tsx:224-228`: empty lines with orphaned comment `// Single item clipboard copy handler` -- `EventView.tsx:157-158`: empty lines -- `IssuesAndPRsList.tsx:316`: empty blank line -- `IssuesAndPRsList.tsx:93-94`: empty comment block - -These are noise. Clean them up. - ---- - -## LOW: `headerRight = null` Pattern - -Both `Summary.tsx:294` and `EventView.tsx:228` set `const headerRight = null` then pass it as a prop. This is unnecessary ceremony -- just omit the prop or pass `null` inline. - ---- - -## LOW: Test Environment Detection Duplicated - -`src/App.tsx:220-223` and `src/hooks/useLocalStorage.ts:132-135` both duplicate the same test-environment detection logic: -```ts -const isTestEnvironment = typeof window !== 'undefined' && - (window.navigator?.userAgent?.includes('jsdom') || - process.env.NODE_ENV === 'test' || - import.meta.env?.MODE === 'test'); -``` - -Extract to a utility: `isTestEnvironment()`. - ---- - -## LOW: `summaryHelpers.ts` Over-Extraction - -`src/utils/summaryHelpers.ts` contains tiny one-liner functions: -```ts -export const hasAnyItems = (groups) => Object.values(groups).some(items => items.length > 0); -export const isSectionCollapsed = (name, set) => set.has(name); -``` - -`isSectionCollapsed` is literally `Set.has` with a wrapper. `hasAnyItems` is one expression. These add indirection without value. Inline them at the call site. - ---- - -## Summary of Recommendations (by impact) - -| Priority | Issue | Action | -|----------|-------|--------| -| **Critical** | 3 views duplicate selection, dialog, clipboard logic | Extract `useListSelection` and `useDialogNavigation` hooks | -| **High** | `categorizeItem` / `categorizeItemWithoutDateFiltering` near-identical | Merge into one parameterized function | -| **High** | `FormContextType` defined in two places | Consolidate in `types.ts` | -| **High** | ItemRow renders full desktop + mobile DOM | Share sub-components or use responsive CSS | -| **High** | Duplicate utility functions across files | Delete duplicates (`parseUsernames`, `isAuthoredBy...`) | -| **Medium** | `as unknown as` casts for IndexedDB items | Make `useIndexedDBStorage` generic | -| **Medium** | Unreachable code after `throw` | Remove dead lines or fix intent | -| **Medium** | Empty `handleUsernameBlur` | Remove or implement | -| **Medium** | Redundant `length === 0` / `length < 1` checks | Remove duplicate check | -| **Low** | Blank lines, orphan comments, `headerRight = null` | Clean up | diff --git a/src/components/CopyToClipboardButton.tsx b/src/components/CopyToClipboardButton.tsx index e88a758..d811ce4 100644 --- a/src/components/CopyToClipboardButton.tsx +++ b/src/components/CopyToClipboardButton.tsx @@ -5,7 +5,7 @@ import { CheckIcon, } from '@primer/octicons-react'; import { useCopyFeedback } from '../hooks/useCopyFeedback'; -import { GitHubItem } from '../types'; +import { GitHubItem, getItemId } from '../types'; interface CopyToClipboardButtonProps { item: GitHubItem; @@ -27,7 +27,7 @@ const CopyToClipboardButton = memo(function CopyToClipboardButton({ const handleCopy = useCallback(async () => { try { await navigator.clipboard.writeText(item.html_url); - triggerCopy(item.event_id || item.id); + triggerCopy(getItemId(item)); onSuccess?.(); } catch (error) { console.error('Failed to copy link:', error); @@ -35,7 +35,7 @@ const CopyToClipboardButton = memo(function CopyToClipboardButton({ } }, [item, triggerCopy, onSuccess, onError]); - const isItemCopied = isCopied(item.event_id || item.id); + const isItemCopied = isCopied(getItemId(item)); return ( onSelect(item.event_id || item.id)} + onChange={() => onSelect(getItemId(item))} sx={{ flexShrink: 0 }} /> )} @@ -274,7 +274,7 @@ const ItemRow = ({ {showCheckbox && onSelect && ( onSelect(item.event_id || item.id)} + onChange={() => onSelect(getItemId(item))} sx={{ flexShrink: 0 }} /> )} diff --git a/src/hooks/useDialogNavigation.ts b/src/hooks/useDialogNavigation.ts index a2af479..bf5131d 100644 --- a/src/hooks/useDialogNavigation.ts +++ b/src/hooks/useDialogNavigation.ts @@ -1,4 +1,4 @@ -import { useState, useCallback } from 'react'; +import { useState, useCallback, useMemo } from 'react'; import { GitHubItem } from '../types'; interface UseDialogNavigationReturn { @@ -19,9 +19,12 @@ export function useDialogNavigation( const [selectedItemForDialog, setSelectedItemForDialog] = useState(null); - const currentIndex = selectedItemForDialog - ? items.findIndex(item => item.id === selectedItemForDialog.id) - : -1; + const currentIndex = useMemo( + () => selectedItemForDialog + ? items.findIndex(item => item.id === selectedItemForDialog.id) + : -1, + [selectedItemForDialog, items], + ); const handlePreviousItem = useCallback(() => { if (currentIndex > 0) { diff --git a/src/hooks/useListSelection.ts b/src/hooks/useListSelection.ts index 336a386..16d2871 100644 --- a/src/hooks/useListSelection.ts +++ b/src/hooks/useListSelection.ts @@ -1,5 +1,5 @@ import { useCallback, useMemo } from 'react'; -import { GitHubItem } from '../types'; +import { GitHubItem, getItemId } from '../types'; import { useLocalStorage } from './useLocalStorage'; interface UseListSelectionReturn { @@ -11,8 +11,6 @@ interface UseListSelectionReturn { selectAllState: { checked: boolean; indeterminate: boolean }; } -const getItemId = (item: GitHubItem): string | number => item.event_id || item.id; - /** * Shared hook for item selection logic used across all view components. * Manages selected items state, select-all checkbox state, and bulk operations. @@ -33,15 +31,15 @@ export function useListSelection( } return newSet; }); - }, []); + }, [setSelectedItems]); const selectAllItems = useCallback(() => { setSelectedItems(new Set(allSelectableItems.map(getItemId))); - }, [allSelectableItems]); + }, [allSelectableItems, setSelectedItems]); const clearSelection = useCallback(() => { setSelectedItems(new Set()); - }, []); + }, [setSelectedItems]); const bulkSelectItems = useCallback((itemIds: (string | number)[], shouldSelect: boolean) => { setSelectedItems(prev => { @@ -53,7 +51,15 @@ export function useListSelection( } return newSet; }); - }, []); + }, [setSelectedItems]); + + // Prune selected items that no longer exist in the selectable list + const validSelectedItems = useMemo(() => { + if (selectedItems.size === 0) return selectedItems; + const validIds = new Set(allSelectableItems.map(getItemId)); + const pruned = new Set([...selectedItems].filter(id => validIds.has(id))); + return pruned.size === selectedItems.size ? selectedItems : pruned; + }, [allSelectableItems, selectedItems]); const selectAllState = useMemo(() => { if (allSelectableItems.length === 0) { @@ -61,7 +67,7 @@ export function useListSelection( } const selectedCount = allSelectableItems.filter(item => - selectedItems.has(getItemId(item)) + validSelectedItems.has(getItemId(item)) ).length; if (selectedCount === 0) { @@ -71,10 +77,10 @@ export function useListSelection( } else { return { checked: false, indeterminate: true }; } - }, [allSelectableItems, selectedItems]); + }, [allSelectableItems, validSelectedItems]); return { - selectedItems, + selectedItems: validSelectedItems, toggleItemSelection, selectAllItems, clearSelection, diff --git a/src/types.ts b/src/types.ts index d378b45..122ae26 100644 --- a/src/types.ts +++ b/src/types.ts @@ -48,6 +48,9 @@ export interface GitHubItem { originalEventType?: string; // Original GitHub event type (e.g., 'PullRequestReviewEvent') } +/** Returns the unique identifier for a GitHubItem (event_id if present, otherwise id). */ +export const getItemId = (item: GitHubItem): string | number => item.event_id || item.id; + // Raw data storage types export interface RawDataStorage { // Raw events from GitHub Events API (legacy - now stored in IndexedDB) diff --git a/src/utils/__tests__/summaryGrouping.test.ts b/src/utils/__tests__/summaryGrouping.test.ts index 89b2dee..21a20ea 100644 --- a/src/utils/__tests__/summaryGrouping.test.ts +++ b/src/utils/__tests__/summaryGrouping.test.ts @@ -1,7 +1,6 @@ import { describe, it, expect, beforeEach } from 'vitest'; import { categorizeItem, - categorizeItemWithoutDateFiltering, groupSummaryData, isDateInRange, } from '../summaryGrouping'; @@ -138,7 +137,7 @@ describe('categorizeItem - PRs Updated', () => { }); }); -describe('categorizeItemWithoutDateFiltering - For Already Date-Filtered Results', () => { +describe('categorizeItem with applyDateFiltering=false - For Already Date-Filtered Results', () => { const addedReviewPRs = new Set(); const startDate = '2024-01-10'; const endDate = '2024-01-20'; @@ -159,7 +158,7 @@ describe('categorizeItemWithoutDateFiltering - For Already Date-Filtered Results pull_request: { url: 'https://github.com/test/repo/pull/1' }, }; - const result = categorizeItemWithoutDateFiltering(item, addedReviewPRs, startDate, endDate); + const result = categorizeItem(item, addedReviewPRs, startDate, endDate, false); expect(result).toBe(SUMMARY_GROUP_NAMES.PRS_UPDATED); }); @@ -174,7 +173,7 @@ describe('categorizeItemWithoutDateFiltering - For Already Date-Filtered Results user: { login: 'testuser', avatar_url: '', html_url: '' }, }; - const result = categorizeItemWithoutDateFiltering(item, addedReviewPRs, startDate, endDate); + const result = categorizeItem(item, addedReviewPRs, startDate, endDate, false); expect(result).toBe(SUMMARY_GROUP_NAMES.ISSUES_UPDATED); }); @@ -190,7 +189,7 @@ describe('categorizeItemWithoutDateFiltering - For Already Date-Filtered Results user: { login: 'testuser', avatar_url: '', html_url: '' }, }; - const result = categorizeItemWithoutDateFiltering(item, addedReviewPRs, startDate, endDate); + const result = categorizeItem(item, addedReviewPRs, startDate, endDate, false); expect(result).toBe(SUMMARY_GROUP_NAMES.ISSUES_OPENED); }); @@ -206,7 +205,7 @@ describe('categorizeItemWithoutDateFiltering - For Already Date-Filtered Results user: { login: 'testuser', avatar_url: '', html_url: '' }, }; - const result = categorizeItemWithoutDateFiltering(item, addedReviewPRs, startDate, endDate); + const result = categorizeItem(item, addedReviewPRs, startDate, endDate, false); expect(result).toBe(SUMMARY_GROUP_NAMES.ISSUES_OPENED); }); @@ -222,7 +221,7 @@ describe('categorizeItemWithoutDateFiltering - For Already Date-Filtered Results user: { login: 'testuser', avatar_url: '', html_url: '' }, }; - const result = categorizeItemWithoutDateFiltering(item, addedReviewPRs, startDate, endDate); + const result = categorizeItem(item, addedReviewPRs, startDate, endDate, false); expect(result).toBe(SUMMARY_GROUP_NAMES.ISSUES_UPDATED); }); @@ -237,7 +236,7 @@ describe('categorizeItemWithoutDateFiltering - For Already Date-Filtered Results user: { login: 'otheruser', avatar_url: '', html_url: '' }, // Different user }; - const result = categorizeItemWithoutDateFiltering(item, addedReviewPRs, startDate, endDate); + const result = categorizeItem(item, addedReviewPRs, startDate, endDate, false); expect(result).toBe(SUMMARY_GROUP_NAMES.ISSUES_UPDATED); }); }); diff --git a/src/utils/__tests__/summaryHelpers.test.ts b/src/utils/__tests__/summaryHelpers.test.ts index d08a874..9175b2d 100644 --- a/src/utils/__tests__/summaryHelpers.test.ts +++ b/src/utils/__tests__/summaryHelpers.test.ts @@ -1,9 +1,6 @@ import { formatGroupedDataForClipboard, getAllDisplayedItems, - hasAnyItems, - getTotalItemCount, - isSectionCollapsed, getGroupSelectState } from '../summaryHelpers'; import { GitHubItem } from '../../types'; @@ -30,7 +27,7 @@ describe('summaryHelpers', () => { it('should format grouped data and filter out empty groups', () => { const result = formatGroupedDataForClipboard(mockGroups); - + expect(result).toHaveLength(2); expect(result[0]).toEqual({ groupName: 'PRs - opened', @@ -45,7 +42,7 @@ describe('summaryHelpers', () => { it('should filter to selected items when provided', () => { const selectedItems = new Set([1, 3]); const result = formatGroupedDataForClipboard(mockGroups, selectedItems); - + expect(result).toHaveLength(2); expect(result[0].items).toHaveLength(1); expect(result[0].items[0].id).toBe(1); @@ -56,7 +53,7 @@ describe('summaryHelpers', () => { it('should return empty array when no selected items match', () => { const selectedItems = new Set([999]); const result = formatGroupedDataForClipboard(mockGroups, selectedItems); - + expect(result).toHaveLength(0); }); @@ -75,7 +72,7 @@ describe('summaryHelpers', () => { }; const result = getAllDisplayedItems(mockGroups); - + expect(result).toHaveLength(3); expect(result.map(item => item.id)).toEqual([1, 2, 3]); }); @@ -86,63 +83,6 @@ describe('summaryHelpers', () => { }); }); - describe('hasAnyItems', () => { - it('should return true when groups have items', () => { - const mockGroups = { - 'Group 1': [createMockItem()], - 'Group 2': [], - }; - - expect(hasAnyItems(mockGroups)).toBe(true); - }); - - it('should return false when no groups have items', () => { - const mockGroups = { - 'Group 1': [], - 'Group 2': [], - }; - - expect(hasAnyItems(mockGroups)).toBe(false); - }); - - it('should return false for empty groups object', () => { - expect(hasAnyItems({})).toBe(false); - }); - }); - - describe('getTotalItemCount', () => { - it('should return total count across all groups', () => { - const mockGroups = { - 'Group 1': [createMockItem(), createMockItem()], - 'Group 2': [createMockItem()], - 'Group 3': [], - }; - - expect(getTotalItemCount(mockGroups)).toBe(3); - }); - - it('should return 0 for empty groups', () => { - expect(getTotalItemCount({})).toBe(0); - }); - }); - - describe('isSectionCollapsed', () => { - it('should return true if section is in collapsed set', () => { - const collapsedSections = new Set(['Section 1', 'Section 2']); - expect(isSectionCollapsed('Section 1', collapsedSections)).toBe(true); - }); - - it('should return false if section is not in collapsed set', () => { - const collapsedSections = new Set(['Section 1']); - expect(isSectionCollapsed('Section 2', collapsedSections)).toBe(false); - }); - - it('should return false for empty collapsed set', () => { - const collapsedSections = new Set(); - expect(isSectionCollapsed('Section 1', collapsedSections)).toBe(false); - }); - }); - describe('getGroupSelectState', () => { const mockItems = [ createMockItem({ id: 1 }), @@ -178,9 +118,9 @@ describe('summaryHelpers', () => { createMockItem({ event_id: 'event-2' }), ]; const selectedItems = new Set(['event-1']); - + const result = getGroupSelectState(itemsWithEventId, selectedItems); expect(result).toEqual({ checked: false, indeterminate: true }); }); }); -}); \ No newline at end of file +}); diff --git a/src/utils/environment.ts b/src/utils/environment.ts index 21dadcb..6cc141b 100644 --- a/src/utils/environment.ts +++ b/src/utils/environment.ts @@ -2,7 +2,8 @@ * Detects if the code is running in a test environment (jsdom, vitest, jest). */ export const isTestEnvironment = (): boolean => - typeof window !== 'undefined' && - (window.navigator?.userAgent?.includes('jsdom') || - process.env.NODE_ENV === 'test' || - import.meta.env?.MODE === 'test'); + (typeof window !== 'undefined' && + window.navigator?.userAgent?.includes('jsdom')) || + (typeof process !== 'undefined' && + process.env?.NODE_ENV === 'test') || + import.meta.env?.MODE === 'test'; diff --git a/src/utils/summaryGrouping.ts b/src/utils/summaryGrouping.ts index e6bb00c..7c098b7 100644 --- a/src/utils/summaryGrouping.ts +++ b/src/utils/summaryGrouping.ts @@ -138,14 +138,6 @@ export const categorizeItem = ( return null; }; -// Keep the old name as a re-export for backward compatibility with tests -export const categorizeItemWithoutDateFiltering = ( - item: GitHubItem, - addedReviewPRs: Set, - startDate: string, - endDate: string, -): SummaryGroupName | null => categorizeItem(item, addedReviewPRs, startDate, endDate, false); - /** * Groups GitHub items into summary categories */ diff --git a/src/utils/summaryHelpers.ts b/src/utils/summaryHelpers.ts index 4ecb852..04e7044 100644 --- a/src/utils/summaryHelpers.ts +++ b/src/utils/summaryHelpers.ts @@ -1,4 +1,4 @@ -import { GitHubItem } from '../types'; +import { GitHubItem, getItemId } from '../types'; /** * Creates a formatted group data structure for clipboard operations @@ -14,12 +14,11 @@ export const formatGroupedDataForClipboard = ( items, })); - // Filter to only selected items if any are selected if (selectedItems && selectedItems.size > 0) { groupedData = groupedData .map(({ groupName, items }) => ({ groupName, - items: items.filter(item => selectedItems.has(item.event_id || item.id)), + items: items.filter(item => selectedItems.has(getItemId(item))), })) .filter(({ items }) => items.length > 0); } @@ -34,30 +33,6 @@ export const getAllDisplayedItems = (actionGroups: Record) return Object.values(actionGroups).flat(); }; -/** - * Checks if any groups have items - */ -export const hasAnyItems = (actionGroups: Record): boolean => { - return Object.values(actionGroups).some(items => items.length > 0); -}; - -/** - * Gets total count of all items across groups - */ -export const getTotalItemCount = (actionGroups: Record): number => { - return Object.values(actionGroups).reduce((total, items) => total + items.length, 0); -}; - -/** - * Checks if a section should be collapsed based on stored preferences - */ -export const isSectionCollapsed = ( - sectionName: string, - collapsedSections: Set -): boolean => { - return collapsedSections.has(sectionName); -}; - /** * Gets the select all state for a specific group */ @@ -69,8 +44,8 @@ export const getGroupSelectState = ( return { checked: false, indeterminate: false }; } - const selectedCount = groupItems.filter(item => - selectedItems.has(item.event_id || item.id) + const selectedCount = groupItems.filter(item => + selectedItems.has(getItemId(item)) ).length; if (selectedCount === 0) { @@ -80,4 +55,4 @@ export const getGroupSelectState = ( } else { return { checked: false, indeterminate: true }; } -}; \ No newline at end of file +}; diff --git a/src/views/EventView.tsx b/src/views/EventView.tsx index 0325f93..3f631a0 100644 --- a/src/views/EventView.tsx +++ b/src/views/EventView.tsx @@ -1,11 +1,11 @@ -import { memo, useEffect } from 'react'; +import { memo, useCallback, useEffect } from 'react'; import { Text, Checkbox, Box, Pagination, } from '@primer/react'; -import { GitHubItem, GitHubEvent } from '../types'; +import { GitHubItem, GitHubEvent, getItemId } from '../types'; import { ResultsContainer } from '../components/ResultsContainer'; import { useCopyFeedback } from '../hooks/useCopyFeedback'; @@ -59,7 +59,7 @@ const EventView = memo(function EventView({ // Reset to first page when search changes useEffect(() => { setCurrentPage(1); - }, [searchText]); + }, [searchText, setCurrentPage]); // Pagination const totalPages = Math.ceil(sortedItems.length / itemsPerPage); @@ -67,11 +67,11 @@ const EventView = memo(function EventView({ const paginatedItems = sortedItems.slice(startIndex, startIndex + itemsPerPage); // Copy handler - const copyResultsToClipboard = async (format: 'detailed' | 'compact') => { + const copyResultsToClipboard = useCallback(async (format: 'detailed' | 'compact') => { const selectedItemsArray = selectedItems.size > 0 ? sortedItems.filter((item: GitHubItem) => - selectedItems.has(item.event_id || item.id) + selectedItems.has(getItemId(item)) ) : sortedItems; @@ -80,7 +80,7 @@ const EventView = memo(function EventView({ onSuccess: () => triggerCopy(format), onError: (error: Error) => console.error('Failed to copy results:', error), }); - }; + }, [selectedItems, sortedItems, triggerCopy]); // Select all toggle const handleSelectAllChange = () => { @@ -124,7 +124,6 @@ const EventView = memo(function EventView({ /> } - headerRight={null} className="timeline-view" > @@ -146,7 +145,7 @@ const EventView = memo(function EventView({ key={`${item.id}-${index}`} item={item} onShowDescription={setSelectedItemForDialog} - selected={selectedItems.has(item.event_id || item.id)} + selected={selectedItems.has(getItemId(item))} onSelect={toggleItemSelection} showCheckbox={true} showRepo={true} diff --git a/src/views/IssuesAndPRsList.tsx b/src/views/IssuesAndPRsList.tsx index 1d3ea33..bf9679e 100644 --- a/src/views/IssuesAndPRsList.tsx +++ b/src/views/IssuesAndPRsList.tsx @@ -16,7 +16,7 @@ import { ChevronUpIcon, } from '@primer/octicons-react'; -import { GitHubItem } from '../types'; +import { GitHubItem, getItemId } from '../types'; import { useCopyFeedback } from '../hooks/useCopyFeedback'; import { useListSelection } from '../hooks/useListSelection'; @@ -25,6 +25,7 @@ import { useFormContext } from '../App'; import { copyResultsToClipboard as copyToClipboard } from '../utils/clipboard'; import { sortItemsByUpdatedDate, filterItemsByAdvancedSearch } from '../utils/viewFiltering'; +import { getGroupSelectState } from '../utils/summaryHelpers'; import { ResultsContainer } from '../components/ResultsContainer'; import DescriptionDialog from '../components/DescriptionDialog'; @@ -56,6 +57,56 @@ const DialogTitle = ({ item }: { item: GitHubItem }) => ( ); +const SectionContent = ({ + groupName, groupItems, itemsPerPage, getSectionPage, setSectionPage, + selectedItems, toggleItemSelection, setSelectedItemForDialog, +}: { + groupName: string; + groupItems: GitHubItem[]; + itemsPerPage: number; + getSectionPage: (name: string) => number; + setSectionPage: (name: string, page: number) => void; + selectedItems: Set; + toggleItemSelection: (id: string | number) => void; + setSelectedItemForDialog: (item: GitHubItem | null) => void; +}) => { + const sectionPage = getSectionPage(groupName); + const sectionTotalPages = Math.ceil(groupItems.length / itemsPerPage); + const startIndex = (sectionPage - 1) * itemsPerPage; + const paginatedGroupItems = groupItems.slice(startIndex, startIndex + itemsPerPage); + + return ( +
+ {paginatedGroupItems.map((item: GitHubItem) => ( +
+ +
+ ))} + {sectionTotalPages > 1 && ( + + setSectionPage(groupName, page)} + showPages={{ narrow: false }} + marginPageCount={2} + surroundingPageCount={2} + /> + + )} +
+ ); +}; + const IssuesAndPRsList = memo(function IssuesAndPRsList({ results, }: { @@ -75,12 +126,12 @@ const IssuesAndPRsList = memo(function IssuesAndPRsList({ const setSectionPage = useCallback((sectionName: string, page: number) => { setSectionPages(prev => ({ ...prev, [sectionName]: page })); - }, []); + }, [setSectionPages]); // Reset pagination when search changes useEffect(() => { setSectionPages({}); - }, [searchText]); + }, [searchText, setSectionPages]); const { isCopied, triggerCopy } = useCopyFeedback(2000); @@ -135,7 +186,7 @@ const IssuesAndPRsList = memo(function IssuesAndPRsList({ newSet.add(sectionName); const sectionItems = groupedItems[sectionName as keyof typeof groupedItems] || []; if (sectionItems.length > 0) { - const idsToRemove = sectionItems.map(item => item.event_id || item.id); + const idsToRemove = sectionItems.map(item => getItemId(item)); bulkSelectItems(idsToRemove, false); } } @@ -154,7 +205,7 @@ const IssuesAndPRsList = memo(function IssuesAndPRsList({ finalGroupedData = groupedData .map(group => ({ ...group, - items: group.items.filter(item => selectedItems.has(item.event_id || item.id)), + items: group.items.filter(item => selectedItems.has(getItemId(item))), })) .filter(group => group.items.length > 0); } @@ -162,7 +213,7 @@ const IssuesAndPRsList = memo(function IssuesAndPRsList({ const allItems = Object.values(groupedItems).flat(); const selectedItemsArray = selectedItems.size > 0 - ? allItems.filter(item => selectedItems.has(item.event_id || item.id)) + ? allItems.filter(item => selectedItems.has(getItemId(item))) : allItems; await copyToClipboard(selectedItemsArray, { @@ -209,7 +260,6 @@ const IssuesAndPRsList = memo(function IssuesAndPRsList({ /> } - headerRight={null} >
{allDisplayedItems.length === 0 ? ( @@ -229,20 +279,12 @@ const IssuesAndPRsList = memo(function IssuesAndPRsList({ { - const sectionItemIds = groupItems.map(item => item.event_id || item.id); - return sectionItemIds.length > 0 && sectionItemIds.every(id => selectedItems.has(id)); - })()} - indeterminate={(() => { - const sectionItemIds = groupItems.map(item => item.event_id || item.id); - const selectedCount = sectionItemIds.filter(id => selectedItems.has(id)).length; - return selectedCount > 0 && selectedCount < sectionItemIds.length; - })()} + {...getGroupSelectState(groupItems, selectedItems)} onChange={() => { if (collapsedSections.has(groupName)) return; - const sectionItemIds = groupItems.map(item => item.event_id || item.id); - const selectedCount = sectionItemIds.filter(id => selectedItems.has(id)).length; - bulkSelectItems(sectionItemIds, selectedCount !== sectionItemIds.length); + const sectionItemIds = groupItems.map(getItemId); + const allSelected = sectionItemIds.every(id => selectedItems.has(id)); + bulkSelectItems(sectionItemIds, !allSelected); }} sx={{ flexShrink: 0 }} aria-label={`Select all items in ${groupName} section`} @@ -269,42 +311,18 @@ const IssuesAndPRsList = memo(function IssuesAndPRsList({
- {!collapsedSections.has(groupName) && (() => { - const sectionPage = getSectionPage(groupName); - const sectionTotalPages = Math.ceil(groupItems.length / itemsPerPage); - const startIndex = (sectionPage - 1) * itemsPerPage; - const paginatedGroupItems = groupItems.slice(startIndex, startIndex + itemsPerPage); - return ( -
- {paginatedGroupItems.map((item: GitHubItem) => ( -
- -
- ))} - {sectionTotalPages > 1 && ( - - setSectionPage(groupName, page)} - showPages={{ narrow: false }} - marginPageCount={2} - surroundingPageCount={2} - /> - - )} -
- ); - })()} + {!collapsedSections.has(groupName) && ( + + )}
); })} diff --git a/src/views/Summary.tsx b/src/views/Summary.tsx index 319a925..08ce445 100644 --- a/src/views/Summary.tsx +++ b/src/views/Summary.tsx @@ -11,7 +11,7 @@ import { ChevronDownIcon, ChevronUpIcon, } from '@primer/octicons-react'; -import { GitHubItem, GitHubEvent } from '../types'; +import { GitHubItem, GitHubEvent, getItemId } from '../types'; import { ResultsContainer } from '../components/ResultsContainer'; import { copyResultsToClipboard as copyToClipboard } from '../utils/clipboard'; @@ -41,6 +41,32 @@ interface SummaryProps { indexedDBSearchItems?: GitHubItem[]; } +/** Returns the most recently updated item from a group. */ +const getMostRecent = (items: GitHubItem[]): GitHubItem => + items.reduce((latest, current) => + new Date(current.updated_at) > new Date(latest.updated_at) ? current : latest + ); + +/** Groups items by URL, deduplicating comments and separating reviewers. */ +const groupItemsByUrl = (groupItems: GitHubItem[]): Record => { + const urlGroups: Record = {}; + groupItems.forEach(item => { + let groupingKey = item.html_url; + if (getEventType(item) === 'comment') { + groupingKey = groupingKey.split('#')[0]; + } + const isReview = (item.title && item.title.startsWith('Review on:')) || item.originalEventType === 'PullRequestReviewEvent'; + if (isReview) { + groupingKey = `${item.user.login}:${groupingKey}`; + } + if (!urlGroups[groupingKey]) { + urlGroups[groupingKey] = []; + } + urlGroups[groupingKey].push(item); + }); + return urlGroups; +}; + const SummaryView = memo(function SummaryView({ items, rawEvents = [], @@ -94,7 +120,7 @@ const SummaryView = memo(function SummaryView({ newSet.add(sectionName); const sectionItems = actionGroups[sectionName as keyof typeof actionGroups] || []; if (sectionItems.length > 0) { - const idsToRemove = sectionItems.map(item => item.event_id || item.id); + const idsToRemove = sectionItems.map(item => getItemId(item)); bulkSelectItems(idsToRemove, false); } } @@ -108,7 +134,7 @@ const SummaryView = memo(function SummaryView({ const allItems = getAllDisplayedItems(actionGroups); const selectedItemsArray = selectedItems.size > 0 - ? allItems.filter(item => selectedItems.has(item.event_id || item.id)) + ? allItems.filter(item => selectedItems.has(getItemId(item))) : allItems; await copyToClipboard(selectedItemsArray, { @@ -158,7 +184,6 @@ const SummaryView = memo(function SummaryView({ /> } - headerRight={null} className="timeline-view" > @@ -177,22 +202,10 @@ const SummaryView = memo(function SummaryView({ ) : ( Object.entries(actionGroups).map(([groupName, groupItems]) => { if (groupItems.length === 0) return null; - // Group items by URL (for reviews, include user to allow multiple reviewers per PR) - const urlGroups: { [url: string]: GitHubItem[] } = {}; - groupItems.forEach(item => { - let groupingKey = item.html_url; - if (getEventType(item) === 'comment') { - groupingKey = groupingKey.split('#')[0]; - } - const isReview = (item.title && item.title.startsWith('Review on:')) || item.originalEventType === 'PullRequestReviewEvent'; - if (isReview) { - groupingKey = `${item.user.login}:${groupingKey}`; - } - if (!urlGroups[groupingKey]) { - urlGroups[groupingKey] = []; - } - urlGroups[groupingKey].push(item); - }); + const urlGroups = groupItemsByUrl(groupItems); + const mostRecentIds = Object.values(urlGroups).map(items => getItemId(getMostRecent(items))); + const selectedCount = mostRecentIds.filter(id => selectedItems.has(id)).length; + const isCollapsed = collapsedSections.has(groupName); return (
@@ -202,40 +215,21 @@ const SummaryView = memo(function SummaryView({ { - if (collapsedSections.has(groupName)) return; - const sectionItemIds = Object.values(urlGroups).map(items => { - const mostRecent = items.reduce((latest, current) => - new Date(current.updated_at) > new Date(latest.updated_at) ? current : latest - ); - return mostRecent.event_id || mostRecent.id; - }); - const selectedCount = sectionItemIds.filter(id => selectedItems.has(id)).length; - bulkSelectItems(sectionItemIds, selectedCount !== sectionItemIds.length); + if (isCollapsed) return; + bulkSelectItems(mostRecentIds, selectedCount !== mostRecentIds.length); }} sx={{ flexShrink: 0 }} aria-label={`Select all events in ${groupName} section`} - disabled={collapsedSections.has(groupName)} + disabled={isCollapsed} /> {groupName} - {(() => { - const totalCount = Object.keys(urlGroups).length; - const sectionItemIds = Object.values(urlGroups).map(items => { - const mostRecent = items.reduce((latest, current) => - new Date(current.updated_at) > new Date(latest.updated_at) ? current : latest - ); - return mostRecent.event_id || mostRecent.id; - }); - const selectedCount = sectionItemIds.filter(id => selectedItems.has(id)).length; - return ( - 0 ? `${selectedCount} / ${totalCount}` : `${totalCount}`} - size="small" - sx={{ ml: 2, flexShrink: 0 }} - /> - ); - })()} + 0 ? `${selectedCount} / ${mostRecentIds.length}` : `${mostRecentIds.length}`} + size="small" + sx={{ ml: 2, flexShrink: 0 }} + />
- {!collapsedSections.has(groupName) && ( + {!isCollapsed && (
{Object.entries(urlGroups).map(([url, items]) => { - const mostRecent = items.reduce((latest, current) => - new Date(current.updated_at) > new Date(latest.updated_at) ? current : latest - ); + const mostRecent = getMostRecent(items); return (
1 ? items.length : undefined} diff --git a/src/views/__tests__/IssuesAndPRsList.test.tsx b/src/views/__tests__/IssuesAndPRsList.test.tsx index 87d02c0..a3969f5 100644 --- a/src/views/__tests__/IssuesAndPRsList.test.tsx +++ b/src/views/__tests__/IssuesAndPRsList.test.tsx @@ -45,7 +45,7 @@ const generateItems = (count: number, isPR: boolean, startId = 1): GitHubItem[] const renderComponent = (results: GitHubItem[]) => render( - + );