docs: Add comprehensive code review for readability and maintainability

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 |

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(),