fix(github): use selectedPR from hook to restore Files changed list #822
Conversation
The hook useGitHubPRs returns a selectedPR that includes full PR details including the files array and changedFiles count. GitHubPRs.tsx was ignoring this and doing its own lookup in the prs array (which only contains list-view PRs without file details). This caused the Files changed list to appear empty in the PR detail view. Fixes ACS-173
📝 WalkthroughWalkthroughFrontend GitHub/GitLab PR and issue components and hooks now consume hook-provided selectedPR/selectedIssue values, add new hook return fields (e.g., isLoadingPRDetails), introduce race-guarded PR detail fetching and investigation flows, and normalize string-literal quoting; IPC GitHub PR handler exported literal unions were normalized to double-quoted strings. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as GitHubPRs UI
participant Hook as useGitHubPRs
participant IPC as IPC / Main handler
participant Store as Renderer Store
User->>UI: select PR
UI->>Hook: setSelectedPRNumber / request details
Hook->>IPC: fetch PR details (with race guard)
IPC-->>Hook: return PR details
Hook->>Store: update PR details / reviews
Store-->>UI: selectedPR / isLoadingPRDetails updated
UI->>UI: render PRDetail (uses hook-provided selectedPR)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @StillKnotKnown, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a display bug in the GitHub PR detail view, where file change information was missing. The fix involves correcting the data source for the selected pull request, ensuring that the component now properly accesses and renders the full details, including the file list and change count, by leveraging the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug where the 'Files changed' list was not appearing in the PR detail view. The root cause was accurately identified as using PR data from the list view (prs array) instead of the detailed PR data (selectedPR) which is already fetched and provided by the useGitHubPRs hook. The fix is simple and effective, replacing the local lookup with the selectedPR from the hook. The accompanying formatting changes across the file are consistent and improve readability. The change is well-described and the fix is solid. No issues were found in this pull request.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx (1)
15-23: Explicit typing improves type safety and testability.The explicit
tprop typing is a good improvement. The component is now more testable by allowing the translation function to be injected.💡 Optional: Consider using the TFunction type from react-i18next
For stronger type safety and better compatibility with react-i18next's features (interpolation, pluralization, etc.), you could use the
TFunctiontype:+import { TFunction } from "react-i18next"; + function NotConnectedState({ error, onOpenSettings, t, }: { error: string | null; onOpenSettings?: () => void; - t: (key: string) => string; + t: TFunction<"common">; }) {However, the current simpler signature is perfectly adequate for this use case.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx
🧠 Learnings (3)
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/frontend/src/**/*.{ts,tsx,jsx} : Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Applied to files:
apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/frontend/src/**/*.{ts,tsx} : Use `useTranslation()` hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Applied to files:
apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx
📚 Learning: 2026-01-04T23:59:45.209Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:45.209Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings) can be deferred for future i18n cleanup passes. Do not fix such issues in PRs that do not introduce new i18n violations, especially in frontend TSX components (e.g., apps/frontend/**/*.tsx). If a PR adds new i18n violations, address them in that PR.
Applied to files:
apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx
🧬 Code graph analysis (1)
apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx (1)
apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts (1)
useGitHubPRs(48-425)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx (2)
83-83: LGTM! This is the critical fix for the missing files list.Using
selectedPRdirectly from theuseGitHubPRshook is the correct solution. According to the hook implementation,selectedPRprioritizesselectedPRDetails(which includes full PR data with files and changedFiles count) over the list-view PR entries in theprsarray. This directly resolves the "File list not available" bug by ensuring the PR detail view receives complete file information.
240-263: Correct usage of selectedPR with proper loading state handling.The conditional rendering correctly handles the null case for
selectedPR, and theisLoadingPRDetailsflag is passed toPRDetailasisLoadingFiles, which ensures the component can display appropriate loading states while file details are being fetched.🔍 Optional: Verify PRDetail handles partial PR data gracefully
While the loading flag is passed correctly, you may want to verify that the
PRDetailcomponent handles cases whereselectedPRmight be from theprsarray (without full file details) beforeselectedPRDetailsfinishes loading. Run a quick manual test:Test steps:
- Open the GitHub PRs view
- Click a PR immediately (before details load)
- Verify that no errors occur and a loading state is shown for the files section
- Wait for files to load and verify they appear correctly
This should work correctly given the
isLoadingFilesprop, but manual verification ensures a smooth user experience during the loading transition.
StillKnotKnown
added
bug
The GitHub API may return null for additions, deletions, and changed_files fields in certain edge cases (e.g., draft PRs, PRs with no diff yet). Add null-safe fallbacks (?? 0) to ensure the frontend always receives numeric values instead of null. Also added debug logging to inspect the raw API response for troubleshooting. Related to ACS-173
StillKnotKnown
left a comment
StillKnotKnown
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 3 issue(s) require attention.
3 issue(s) must be addressed (0 required, 3 recommended)
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- Medium: 3 issue(s)
Generated by Auto Claude PR Review
Findings (3 selected of 3 total)
🟡 [e9a44ba041db] [MEDIUM] Pre-existing: UI flicker when selecting PR due to immediate clearing of previous details
📁 apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts:106
The hook's selectPR function immediately clears selectedPRDetails (line 220) before fetching new data, causing selectedPR to be temporarily null even when selectedPRNumber is set. This can cause the PR detail panel to briefly show 'Select a PR to view' immediately after clicking a PR, especially when the PR list is filtered. This is a pre-existing issue in the hook's design, not introduced by this PR.
Suggested fix:
Preserve previous PR details until new data arrives: `setSelectedPRDetails(prev => prev?.number !== prNumber ? null : prev)` to show stale data during fetch instead of empty state
🟡 [dd140fe3e0b4] [MEDIUM] Related: Inconsistent pattern - manual lookup instead of using hook-provided data
📁 apps/frontend/src/renderer/components/GitLabIssues.tsx:77
This component manually finds the selected issue with issues.find(i => i.iid === selectedIssueIid), similar to the pattern that this PR fixes in GitHubPRs.tsx. For consistency, the useGitLabIssues hook should provide selectedIssue just like useGitHubPRs provides selectedPR. This is a related code improvement, not a blocker for this PR.
Suggested fix:
Update useGitLabIssues hook to return selectedIssue (similar to line 106 in useGitHubPRs.ts), then use it directly in the component instead of manual lookup
🟡 [d4ce2044b808] [MEDIUM] Related: Inconsistent pattern - manual lookup instead of using hook-provided data
📁 apps/frontend/src/renderer/components/GitHubIssues.tsx:118
This component manually finds the selected issue with issues.find(i => i.number === selectedIssueNumber), similar to the pattern that this PR fixes in GitHubPRs.tsx. For consistency, the useGitHubIssues hook should provide selectedIssue just like useGitHubPRs provides selectedPR. This is a related code improvement, not a blocker for this PR.
Suggested fix:
Update useGitHubIssues hook to return selectedIssue (similar to line 106 in useGitHubPRs.ts), then use it directly in the component instead of manual lookup
This review was generated by Auto Claude.
This addresses PR review findings about inconsistent patterns: 1. Fix UI flicker in useGitHubPRs hook - Don't clear previous PR details when switching PRs - Preserve previous details during fetch to avoid empty state 2. Add selectedIssue to useGitLabIssues hook - Return computed selectedIssue instead of manual lookup - Update GitLabIssues.tsx to use hook-provided value 3. Add selectedIssue to useGitHubIssues hook - Return computed selectedIssue instead of manual lookup - Update GitHubIssues.tsx to use hook-provided value Related to ACS-173
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| data.posted_at = new Date().toISOString(); | ||
| fs.writeFileSync(reviewPath, JSON.stringify(data, null, 2), 'utf-8'); | ||
| debugLog('Updated review result with review ID and posted findings', { prNumber, reviewId, postedCount: newPostedIds.length }); | ||
| fs.writeFileSync(reviewPath, JSON.stringify(data, null, 2), "utf-8"); |
Check warning
Code scanning / CodeQL
Network data written to file Medium
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts (1)
109-120: Align quote style in PRReviewFinding type definition across files.The
severityandcategorytype unions in this file now use double quotes ("critical" | "high"), but the same interface inapps/frontend/src/preload/api/modules/github-api.ts(lines 322-325) uses single quotes ('critical' | 'high'). While TypeScript treats these equivalently, unify the quote style for consistency. Update either both files to double quotes or both to single quotes to match your codebase conventions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
apps/frontend/src/main/ipc-handlers/github/pr-handlers.tsapps/frontend/src/renderer/components/GitHubIssues.tsxapps/frontend/src/renderer/components/GitLabIssues.tsxapps/frontend/src/renderer/components/github-issues/hooks/useGitHubIssues.tsapps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.tsapps/frontend/src/renderer/components/gitlab-issues/hooks/useGitLabIssues.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/renderer/components/github-issues/hooks/useGitHubIssues.tsapps/frontend/src/renderer/components/GitHubIssues.tsxapps/frontend/src/renderer/components/gitlab-issues/hooks/useGitLabIssues.tsapps/frontend/src/renderer/components/GitLabIssues.tsxapps/frontend/src/main/ipc-handlers/github/pr-handlers.tsapps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/renderer/components/github-issues/hooks/useGitHubIssues.tsapps/frontend/src/renderer/components/GitHubIssues.tsxapps/frontend/src/renderer/components/gitlab-issues/hooks/useGitLabIssues.tsapps/frontend/src/renderer/components/GitLabIssues.tsxapps/frontend/src/main/ipc-handlers/github/pr-handlers.tsapps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/renderer/components/github-issues/hooks/useGitHubIssues.tsapps/frontend/src/renderer/components/GitHubIssues.tsxapps/frontend/src/renderer/components/gitlab-issues/hooks/useGitLabIssues.tsapps/frontend/src/renderer/components/GitLabIssues.tsxapps/frontend/src/main/ipc-handlers/github/pr-handlers.tsapps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts
🧠 Learnings (2)
📚 Learning: 2026-01-04T23:59:45.209Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:45.209Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings) can be deferred for future i18n cleanup passes. Do not fix such issues in PRs that do not introduce new i18n violations, especially in frontend TSX components (e.g., apps/frontend/**/*.tsx). If a PR adds new i18n violations, address them in that PR.
Applied to files:
apps/frontend/src/renderer/components/GitHubIssues.tsxapps/frontend/src/renderer/components/GitLabIssues.tsx
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/frontend/src/**/*.{ts,tsx} : Use `useTranslation()` hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Applied to files:
apps/frontend/src/renderer/components/GitLabIssues.tsx
🧬 Code graph analysis (4)
apps/frontend/src/renderer/components/github-issues/hooks/useGitHubIssues.ts (3)
apps/frontend/src/renderer/components/github-issues/types/index.ts (1)
FilterState(4-4)apps/frontend/src/renderer/stores/github/index.ts (1)
loadGitHubIssues(15-15)apps/frontend/src/renderer/stores/github/issues-store.ts (1)
loadGitHubIssues(86-103)
apps/frontend/src/renderer/components/GitHubIssues.tsx (4)
apps/frontend/src/renderer/components/github-issues/hooks/useGitHubIssues.ts (1)
useGitHubIssues(11-82)apps/frontend/src/renderer/components/github-issues/hooks/useAnalyzePreview.ts (1)
useAnalyzePreview(30-167)apps/frontend/src/renderer/stores/github/index.ts (1)
GitHubIssue(57-57)apps/frontend/src/renderer/components/github-issues/components/IssueListHeader.tsx (1)
IssueListHeader(23-162)
apps/frontend/src/renderer/components/GitLabIssues.tsx (10)
apps/frontend/src/renderer/components/gitlab-issues/index.ts (8)
GitLabIssues(2-2)GitLabIssuesProps(6-6)useGitLabIssues(17-17)useGitLabInvestigation(18-18)useIssueFiltering(19-19)NotConnectedState(28-28)IssueListHeader(29-29)EmptyState(27-27)apps/frontend/src/renderer/components/gitlab-issues/types/index.ts (1)
GitLabIssuesProps(6-10)apps/frontend/src/renderer/components/gitlab-issues/hooks/useGitLabIssues.ts (1)
useGitLabIssues(9-75)apps/frontend/src/renderer/components/gitlab-issues/hooks/index.ts (3)
useGitLabIssues(1-1)useGitLabInvestigation(2-2)useIssueFiltering(3-3)apps/frontend/src/renderer/components/gitlab-issues/hooks/useGitLabInvestigation.ts (1)
useGitLabInvestigation(6-75)apps/frontend/src/renderer/components/gitlab-issues/hooks/useIssueFiltering.ts (1)
useIssueFiltering(5-17)apps/frontend/src/shared/types/integrations.ts (1)
GitLabIssue(168-191)apps/frontend/src/renderer/components/gitlab-issues/components/EmptyStates.tsx (2)
NotConnectedState(21-43)EmptyState(6-19)apps/frontend/src/renderer/components/gitlab-issues/components/index.ts (3)
NotConnectedState(4-4)IssueListHeader(5-5)EmptyState(4-4)apps/frontend/src/renderer/components/gitlab-issues/components/IssueListHeader.tsx (1)
IssueListHeader(15-83)
apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts (2)
apps/frontend/src/preload/api/modules/github-api.ts (3)
PRReviewFinding(322-333)PRReviewResult(338-360)PRData(296-317)apps/frontend/src/shared/constants/models.ts (1)
MODEL_ID_MAP(19-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (27)
apps/frontend/src/renderer/components/github-issues/hooks/useGitHubIssues.ts (4)
1-9: LGTM! Import additions support the new selectedIssue computation.The addition of
useMemoand type imports (IssueFilterState,FilterState) are necessary for the new derived state logic and improve type safety.
53-61: LGTM! Proper useCallback memoization with correct dependencies.The refactor wraps
handleFilterChangeinuseCallbackwith appropriate dependencies.loadGitHubIssuesis correctly excluded from the dependency array since it's a stable module-level import from the store.
63-66: LGTM! Excellent centralization of selection logic.The
selectedIssuecomputation follows best practices:
- Properly memoized with correct dependencies
- Explicit null fallback for clarity
- Aligns with the PR objective to standardize the "selected item" pattern across hooks
This prevents components from deriving the selected issue themselves, reducing duplication and potential UI flicker.
68-81: LGTM! Return object properly extended with selectedIssue.The hook now exposes
selectedIssueas part of its public API, allowing consuming components to access the selected issue directly. This maintains backward compatibility while enabling the standardized selection pattern described in the PR objectives.apps/frontend/src/renderer/components/GitHubIssues.tsx (6)
1-43: LGTM! Hook-provided selectedIssue improves consistency.The addition of
selectedIssuefrom theuseGitHubIssueshook (line 36) aligns with the PR's goal of standardizing the "selected item" pattern. This approach prevents UI flicker and ensures the component consumes the hook's computed value rather than duplicating the selection logic locally.
75-75: Empty string fallback is safe.The fallback to an empty string when
selectedProject?.idis undefined is consistent with the pattern used throughout the component (e.g., line 205 forBatchReviewWizard). The hooks are designed to handle this gracefully.
90-98: Efficient task linking with proper memoization.The
issueToTaskMapprovides O(1) lookup for linking GitHub issues to tasks (used in line 178). The memo dependencies are correct, and this enhances the issue detail view by surfacing linked task information.
101-107: Well-integrated auto-fix refresh enhancement.The
handleRefreshWithAutoFixcallback appropriately extends the refresh behavior to check for new auto-fix issues when enabled. The conditional logic and dependencies are correct.
109-121: Robust investigation flow with proper null safety.The investigation handlers are well-structured:
handleInvestigatecaptures the issue and opens the dialoghandleStartInvestigationincludes defensive null checking before invoking the hook'sstartInvestigation- Dependencies are complete and accurate
169-183: Correct usage of hook-provided selectedIssue.The component now properly consumes
selectedIssuefrom the hook (line 169) rather than computing it locally. This aligns with the PR's standardization goal and prevents potential inconsistencies. ThelinkedTaskIdlookup viaissueToTaskMap(line 178) is also well-integrated.apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts (2)
970-972: Good: Null-safe fallbacks for GitHub API responses.The null-coalescing operators (
?? 0) foradditions,deletions, andchangedFilesprotect against null values from the GitHub API. This defensive coding aligns with the PR objectives mentioning these fallbacks were added.
1038-1047: Good: Full PR details including files are fetched correctly.The
GITHUB_PR_GEThandler properly fetches both PR metadata and the files list, mapping them to thePRDatastructure with null-safe fallbacks. This is the backend support for the fix - the component now needs to consume this data correctly.apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts (3)
129-129: Core fix: Correctly prioritizes detailed PR data over list data.This line is the key fix for the bug. By preferring
selectedPRDetails(which includes thefilesarray from thegetPRAPI call) over theprs.find()result (list data without file details), the component now receives the full PR data needed to display the "Files changed" section.
246-275: Good pattern: Fetch full PR details on selection without clearing previous state.The comment on lines 259-261 correctly explains that
selectedPRDetailsis not cleared to avoid UI flicker - the previous PR's details remain visible until new data arrives. This provides a better UX during loading.However, there's a subtle issue: when
prNumberisnull(line 253-256),selectedPRDetailsis cleared, but when switching between PRs directly (non-null to non-null), the old PR's files will briefly show for the new PR until the fetch completes.Consider whether the
isLoadingPRDetailsstate is sufficient to handle this UX concern in the consuming component (e.g., showing a loading indicator over the files section while fetching).
73-76: LGTM: New loading state properly exposed and returned.The
isLoadingPRDetailsstate is correctly initialized, managed inselectPR, and returned from the hook. This allows consuming components to show appropriate loading UI while fetching full PR details including files.Also applies to: 487-493
apps/frontend/src/renderer/components/gitlab-issues/hooks/useGitLabIssues.ts (4)
1-6: LGTM: Import updates are correct.The addition of
useMemoto the React imports is necessary for the newselectedIssuecomputation, and the multi-line import formatting improves readability.
46-54: LGTM: Proper memoization of filter handler.The
useCallbackwrapper is appropriate for a function returned from a hook, preventing unnecessary re-renders in consuming components. The dependency array correctly includesprojectIdandsetFilterState.
56-59: LGTM: Correct implementation of selectedIssue computation.The
useMemooptimization prevents unnecessary lookups when neither the issues array nor the selected IID changes. The fallback tonullprovides clear semantics for the "no selection" state.
61-74: LGTM: Return value correctly extended with selectedIssue.The hook now provides the computed
selectedIssueto consumers, enabling the component to use the hook's data directly instead of performing its own lookup. This is the key change that prevents stale data issues.apps/frontend/src/renderer/components/GitLabIssues.tsx (8)
1-15: LGTM: Import updates are correct and necessary.All new imports (
useMemo,InvestigationDialog, type imports) are utilized in the component body. The double-quote formatting aligns with the project's coding standards.
24-44: LGTM: Correct usage of selectedIssue from hook.The component now consumes
selectedIssuedirectly from theuseGitLabIssueshook rather than deriving it locally. This is the core fix that prevents stale data issues, matching the pattern described in the PR objectives for GitHub PRs.
48-66: LGTM: Proper state management for investigation dialog.The
selectedIssueForInvestigationstate is correctly typed asGitLabIssue | null, and thehandleInvestigatecallback is appropriately memoized with an empty dependency array since it only references stable state setters.
68-75: LGTM: Correct memoization of investigation handler.The
useCallbackwrapper with dependencies onselectedIssueForInvestigationandstartInvestigationis appropriate. The null-check ensures the investigation only starts when an issue is selected.
83-85: LGTM: Proper use of NotConnectedState component.The component correctly delegates the not-connected UI to the
NotConnectedStatecomponent, which handles i18n internally. The nullish coalescing for the error prop is appropriate.
90-99: LGTM: Correct props passed to IssueListHeader.All required props are correctly wired from the hooks, and the nullish coalescing operator provides a safe empty-string fallback for
projectPathwhen the connection status is unavailable.
117-132: LGTM: Correctly uses selectedIssue from hook (key fix).The component now renders
IssueDetailusingselectedIssuefrom the hook rather than performing a local lookup. This is the core fix that ensures the detail view always displays the full issue data (including files and metadata) fetched by the hook, preventing the "missing files" bug described in the PR objectives.
136-144: LGTM: InvestigationDialog properly integrated.All props are correctly wired to the investigation dialog, including the selected issue state, investigation status from the hook, and appropriate callbacks for starting and closing the investigation flow.
| const reviewedCommitSha = | ||
| result.reviewedCommitSha || (result as any).reviewed_commit_sha; | ||
| if (reviewedCommitSha) { | ||
| window.electronAPI.github | ||
| .checkNewCommits(projectId, prNumber) | ||
| .then((newCommitsResult) => { | ||
| setNewCommitsCheckAction(projectId, prNumber, newCommitsResult); | ||
| }) | ||
| .catch((err) => { | ||
| console.warn(`Failed to check new commits for PR #${prNumber}:`, err); | ||
| }); | ||
| } | ||
| } | ||
| }); | ||
| } else if (existingState?.result) { | ||
| // Review already in store - always check for new commits to get fresh status | ||
| const reviewedCommitSha = | ||
| existingState.result.reviewedCommitSha || | ||
| (existingState.result as any).reviewed_commit_sha; | ||
| if (reviewedCommitSha) { | ||
| window.electronAPI.github | ||
| .checkNewCommits(projectId, prNumber) | ||
| .then((newCommitsResult) => { | ||
| setNewCommitsCheckAction(projectId, prNumber, newCommitsResult); | ||
| }).catch(err => { | ||
| }) | ||
| .catch((err) => { | ||
| console.warn(`Failed to check new commits for PR #${prNumber}:`, err); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Potential redundant API calls for new commits check.
Both branches (lines 291-302 and lines 305-319) call checkNewCommits when a PR is selected. The first branch checks when loading from disk, and the second checks when the review is already in the store. This means every PR selection triggers a new commits check, even if one was just performed moments ago.
While the comment on line 289-290 says "always check... to ensure fresh data," this could cause unnecessary API calls if the user is rapidly clicking between PRs or re-selecting the same PR.
Consider adding a short debounce or TTL cache for the new commits check to avoid redundant API calls within a short time window:
💡 Optional: Add TTL-based caching for new commits check
+// Add a simple TTL check (e.g., skip if checked within last 30 seconds)
+const existingCheck = existingState?.newCommitsCheck;
+const checkAge = existingCheck?.checkedAt
+ ? Date.now() - new Date(existingCheck.checkedAt).getTime()
+ : Infinity;
+const shouldCheckNewCommits = checkAge > 30000; // 30 seconds TTL
+
-if (reviewedCommitSha) {
+if (reviewedCommitSha && shouldCheckNewCommits) {
window.electronAPI.github
.checkNewCommits(projectId, prNumber)Committable suggestion skipped: line range outside the PR's diff.
…-missing-files-changed-list-and-change
|
@StillKnotKnown remember to use the PR template when creating the description (I know that I'm a bit lazy at it aswell :D) |
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 2 issue(s) require attention.
2 issue(s) must be addressed (2 required, 0 recommended), 4 suggestions
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | Low | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- High: 2 issue(s)
- Low: 4 issue(s)
Generated by Auto Claude PR Review
Findings (6 selected of 6 total)
🟠 [f1efc4d200f9] [HIGH] Stale PR data displayed when switching between PRs
📁 apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts:129
When switching from PR #1 to PR #2, selectedPRNumber updates immediately but selectedPRDetails still contains PR #1's data. Since line 129 uses selectedPRDetails || prs.find(...), the old PR #1 data (with its file list) is returned as selectedPR until the async fetch completes. Users will see PR #1's files displayed under PR #2's header for ~200-500ms, causing confusion.
Suggested fix:
Validate that selectedPRDetails matches selectedPRNumber before using it: `const selectedPR = (selectedPRDetails?.number === selectedPRNumber ? selectedPRDetails : null) || prs.find((pr) => pr.number === selectedPRNumber) || null;`
🟠 [1adc622f5212] [HIGH] Race condition: out-of-order API responses when rapidly switching PRs
📁 apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts:263
When user clicks PR #1 then quickly PR #2, two concurrent API calls are made. If PR #1's response arrives AFTER PR #2's response (due to network variability), setSelectedPRDetails(prDetails) will overwrite the correct PR #2 data with stale PR #1 data. There's no staleness check in the .then() callback.
Suggested fix:
Track the current PR number in a ref and check it in the .then() callback: `const currentPRRef = useRef<number | null>(null);` then in selectPR: `currentPRRef.current = prNumber;` and in .then(): `if (prDetails && prNumber === currentPRRef.current) { setSelectedPRDetails(prDetails); }`
🔵 [f696111c6f6d] [LOW] selectedPR not wrapped in useMemo unlike GitHub/GitLab Issues hooks
📁 apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts:129
The PR claims to 'standardize selected item pattern across hooks', but useGitHubPRs uses direct assignment (const selectedPR = ...) while useGitHubIssues and useGitLabIssues use useMemo. This creates inconsistent patterns.
Suggested fix:
Wrap in useMemo for consistency: `const selectedPR = useMemo(() => { return selectedPRDetails || prs.find((pr) => pr.number === selectedPRNumber) || null; }, [selectedPRDetails, prs, selectedPRNumber]);`
🔵 [a97fbbfc35b4] [LOW] GitLab MRs hook missing isLoadingMRDetails state for consistency
📁 apps/frontend/src/renderer/components/gitlab-merge-requests/hooks/useGitLabMRs.ts:95
The useGitHubPRs hook now exposes isLoadingPRDetails state for showing loading indicators while fetching full PR details. The parallel useGitLabMRs hook lacks this, creating inconsistent UX between GitHub and GitLab integrations.
Suggested fix:
Add isLoadingMRDetails and selectedMRDetails state to useGitLabMRs hook following the same pattern as useGitHubPRs
🔵 [f9f5004a5bc1] [LOW] AI comment triage: GitHub Advanced Security alert is false positive
📁 apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts:42
GitHub Advanced Security flagged 'Network data written to file depends on Untrusted data'. This is a FALSE POSITIVE. The sanitizeNetworkData() function (lines 42-81) already implements proper sanitization: removes control characters (0x00-0x08, 0x0B-0x0C, 0x0E-0x1F, 0x7F), limits data to 1MB, and is consistently applied before JSON parsing. No action needed.
Suggested fix:
No change needed. The sanitization is properly implemented.
🔵 [da076ac7d238] [LOW] AI comment triage: CodeRabbit redundant API calls is unfounded
📁 apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts:280
CodeRabbit flagged 'Potential redundant API calls for new commits check' at lines 291-302 and 305-319. This is UNFOUNDED. The two code blocks are in mutually exclusive if-else branches (if (!existingState?.result) vs else if (existingState?.result)). Only ONE branch executes per PR selection. The duplicate code is a minor DRY style concern, not a bug.
Suggested fix:
Optional: Extract checkNewCommits logic into a helper function to reduce code duplication, but this is purely cosmetic.
This review was generated by Auto Claude.
Fixes two HIGH priority issues from PR review:
1. Stale PR data when switching between PRs
- Validate that selectedPRDetails.number matches selectedPRNumber
- Added useMemo wrapper for consistency with other hooks
- Previously, old PR data (with its file list) was briefly shown
under new PR's header until fetch completed
2. Race condition for out-of-order API responses
- Track current PR being fetched in module-level variable
- Only update selectedPRDetails if response matches current PR
- Prevents stale responses from overwriting newer data
Related to ACS-173
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts:
- Around line 219-221: The dependency array for the useCallback that defines
fetchPRs incorrectly includes setNewCommitsCheckAction even though fetchPRs
doesn't reference it; remove setNewCommitsCheckAction from the dependency array
(leaving [projectId, getPRReviewState]) unless you actually intend to call
setNewCommitsCheckAction from fetchPRs—in that case, add the missing call to
setNewCommitsCheckAction inside fetchPRs so the dependency is needed.
- Around line 352-370: Both runReview and runFollowupReview are declared async
but do not await anything; either remove the async modifier or await the store
calls depending on behavior. Inspect storeStartPRReview and
storeStartFollowupReview: if they are fire-and-forget (do not return a Promise)
remove async from runReview and runFollowupReview and keep the early projectId
guard; if they return Promises and callers should wait, change the calls to
await storeStartPRReview(projectId, prNumber) and await
storeStartFollowupReview(projectId, prNumber) and ensure the returned Promise is
propagated by keeping the async signature.
- Around line 18-20: Replace the module-level mutable variable
currentFetchPRNumber with a per-hook ref so instances don't clash: inside the
useGitHubPRs hook create a ref (e.g., currentFetchPRNumberRef via React's
useRef<number | null>(null)) and update all references in selectPR and any
fetch-related logic to read/write currentFetchPRNumberRef.current instead of the
module-scoped currentFetchPRNumber; remove the top-level declaration to
eliminate shared state across hook instances.
- Around line 306-307: Ensure the API response is normalized to camelCase in the
response/handler layer (pr-handlers.ts around the current line ~769) so
reviewed_commit_sha is mapped to reviewedCommitSha (and any similar snake_case
fields are converted) and update the PRReviewResult shape accordingly; then
remove the defensive fallback and the "as any" cast in useGitHubPRs.ts (the
reviewedCommitSha access at the lines with the current fallback at 306-307 and
322-324) so the component reads only result.reviewedCommitSha, and add/update a
small unit or integration test to verify the handler returns camelCased fields.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:45.209Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
🧬 Code graph analysis (1)
apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts (1)
apps/frontend/src/preload/api/modules/github-api.ts (3)
PRReviewProgress(394-399)PRReviewResult(338-360)NewCommitsCheck(365-372)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (4)
apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts (4)
27-67: LGTM!Interface definitions are well-typed. The addition of
isLoadingPRDetailsproperly exposes the loading state for PR file details, andgetReviewStateForPRhas a clear return type.
132-138: Good defensive validation against stale data.The validation that
selectedPRDetails?.number === selectedPRNumberbefore using detailed data prevents showing stale PR information when switching between PRs rapidly. The fallback to list data maintains UI responsiveness.
392-500: LGTM!The callback implementations follow a consistent pattern with proper error handling, store updates, and list refreshes after mutations. The use of
usePRReviewStore.getState()for imperative updates is appropriate for async callbacks.
502-529: LGTM!Return object correctly exposes all state and callbacks matching the
UseGitHubPRsResultinterface.
apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts
Outdated
Show resolved
Hide resolved
| const reviewedCommitSha = | ||
| result.reviewedCommitSha || (result as any).reviewed_commit_sha; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if the file exists and get its line count
file_path="apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts"
if [ -f "$file_path" ]; then
echo "File found. Total lines:"
wc -l "$file_path"
echo ""
echo "Lines 300-315 (context around 306-307):"
sed -n '300,315p' "$file_path"
else
echo "File not found at $file_path"
fiRepository: AndyMik90/Auto-Claude
Length of output: 1069
🏁 Script executed:
#!/bin/bash
# Search for PRReviewResult type definition
echo "=== Searching for PRReviewResult interface/type ==="
rg -n "PRReviewResult" --type=ts | head -20Repository: AndyMik90/Auto-Claude
Length of output: 2636
🏁 Script executed:
#!/bin/bash
# Search for both field naming patterns
echo "=== Searching for reviewed_commit_sha and reviewedCommitSha ==="
rg -n "reviewed_commit_sha|reviewedCommitSha" --type=ts -C 1Repository: AndyMik90/Auto-Claude
Length of output: 11683
🏁 Script executed:
#!/bin/bash
# Get the full PRReviewResult interface definition
sed -n '338,360p' apps/frontend/src/preload/api/modules/github-api.tsRepository: AndyMik90/Auto-Claude
Length of output: 811
Defensive fallback for reviewed_commit_sha suggests inconsistent data normalization upstream.
The fallback (result as any).reviewed_commit_sha at lines 306-307 (and similarly at 322-324) indicates that the API response data is not consistently converted from snake_case to camelCase before being passed to this component. While the PRReviewResult type correctly defines reviewedCommitSha, the code defensively handles both field names because upstream conversion (visible in pr-handlers.ts line 769) may not always occur.
Consider ensuring data is properly normalized to camelCase at the API response handler level to eliminate the need for this fallback and remove the as any assertion.
🤖 Prompt for AI Agents
In @apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts
around lines 306 - 307, Ensure the API response is normalized to camelCase in
the response/handler layer (pr-handlers.ts around the current line ~769) so
reviewed_commit_sha is mapped to reviewedCommitSha (and any similar snake_case
fields are converted) and update the PRReviewResult shape accordingly; then
remove the defensive fallback and the "as any" cast in useGitHubPRs.ts (the
reviewedCommitSha access at the lines with the current fallback at 306-307 and
322-324) so the component reads only result.reviewedCommitSha, and add/update a
small unit or integration test to verify the handler returns camelCased fields.
Fixes 4 issues identified during PR review: 1. Replace module-level mutable variable with per-hook ref - Removed module-level currentFetchPRNumber variable - Added currentFetchPRNumberRef using useRef inside hook - Prevents shared state across hook instances 2. Fix fetchPRs useCallback dependency array - Removed setNewCommitsCheckAction from dependencies - Function doesn't reference it, so it wasn't needed 3. Remove async modifier from fire-and-forget functions - runReview and runFollowupReview don't await anything - Store functions return void, not Promise - Updated interface to reflect void return type 4. Normalize API response to camelCase in handler layer - Updated checkNewCommits handler comment for clarity - Removed defensive fallbacks and "as any" casts in hook - Data is now properly camelCased by the handler Related to ACS-173
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts:
- Around line 249-330: The new-commits check block is duplicated in selectPR;
extract it into a helper like checkNewCommitsForPR(prNumber: number,
reviewedCommitSha?: string) that calls
window.electronAPI.github.checkNewCommits(projectId, prNumber) and then
setNewCommitsCheckAction(projectId, prNumber, newCommitsResult) (with
console.warn on error); call this helper in both places where you currently
check reviewedCommitSha (the branch after loading result from disk and the
branch for existingState.result). Ensure the helper is used inside the same
scope where projectId, setNewCommitsCheckAction and window.electronAPI are
available and referenced from selectPR/current module.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/frontend/src/main/ipc-handlers/github/pr-handlers.tsapps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/ipc-handlers/github/pr-handlers.tsapps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/ipc-handlers/github/pr-handlers.tsapps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/ipc-handlers/github/pr-handlers.tsapps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:45.209Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
📚 Learning: 2026-01-04T23:59:45.209Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:45.209Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Applied to files:
apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (8)
apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts (1)
1-2345: LGTM - Formatting normalization with consistent patterns.The changes in this file are predominantly quote style normalization (single → double quotes) for consistency. The substantive code logic, IPC handler implementations, error handling, and security patterns (sanitization, input validation, command injection prevention) remain intact and properly implemented.
Key observations:
- String literal unions in exported types use consistent double-quoted format
- Null-safe fallbacks (
?? 0,?? "") are properly applied for API responses- Subprocess spawning uses
execFileSyncwith argument arrays (safe from injection)- File operations sanitize network data before parsing
apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts (7)
1-61: LGTM - Clean type definitions and expanded hook interface.The expanded
UseGitHubPRsResultinterface properly documents the newisLoadingPRDetailsstate and the structured return type forgetReviewStateForPR. The return type signatures forrunReviewandrunFollowupReviewcorrectly reflect their fire-and-forget nature.
84-86: Good pattern - Race condition prevention with ref.Using
currentFetchPRNumberRefto track the in-flight PR fetch request is a solid approach to prevent stale data from overwriting current selections when users rapidly switch between PRs.
128-134: Well-designed fallback logic.The validation that
selectedPRDetails?.number === selectedPRNumberbefore using detailed data prevents displaying stale PR details when the user switches PRs before the previous fetch completes. The fallback to list data ensures the UI remains responsive.
136-215: Solid implementation with batched preloading.The
fetchPRscallback properly handles:
- Loading state management with
try/finally- Pagination with deduplication when appending
- Batched review preloading to minimize IPC calls
- Lazy new-commits checking (documented in comment)
343-361: Clean delegation to store actions.The
runReviewandrunFollowupReviewfunctions correctly delegate to store actions (storeStartPRReview,storeStartFollowupReview) which handle both state management and IPC calls. Thevoidreturn type accurately reflects their fire-and-forget nature.
383-491: Proper async error handling and state synchronization.All action callbacks follow a consistent pattern:
- Early return if
!projectIdtry/catchwithsetErroron failure- State/store synchronization on success
The
postReviewcorrectly preservesnewCommitsCheckwhen reloading the review result, andmergePR/assignPRappropriately refresh the PR list after mutations.
493-521: LGTM - Complete return object matching interface.The return object includes all declared properties from
UseGitHubPRsResult, including the newisLoadingPRDetailsstate for consumers to show loading indicators while fetching full PR details.
| const selectPR = useCallback( | ||
| (prNumber: number | null) => { | ||
| setSelectedPRNumber(prNumber); | ||
| // Note: Don't reset review result - it comes from the store now | ||
| // and persists across navigation | ||
|
|
||
| // Clear previous detailed PR data when deselecting | ||
| if (prNumber === null) { | ||
| setSelectedPRDetails(null); | ||
| currentFetchPRNumberRef.current = null; | ||
| return; | ||
| } | ||
|
|
||
| // Clear previous detailed PR data when deselecting | ||
| if (prNumber === null) { | ||
| setSelectedPRDetails(null); | ||
| return; | ||
| } | ||
| if (prNumber && projectId) { | ||
| // Track the current PR being fetched (for race condition prevention) | ||
| currentFetchPRNumberRef.current = prNumber; | ||
|
|
||
| // Fetch full PR details including files | ||
| setIsLoadingPRDetails(true); | ||
| window.electronAPI.github | ||
| .getPR(projectId, prNumber) | ||
| .then((prDetails) => { | ||
| // Only update if this response is still for the current PR (prevents race condition) | ||
| if (prDetails && prNumber === currentFetchPRNumberRef.current) { | ||
| setSelectedPRDetails(prDetails); | ||
| } | ||
| }) | ||
| .catch((err) => { | ||
| console.warn(`Failed to fetch PR details for #${prNumber}:`, err); | ||
| }) | ||
| .finally(() => { | ||
| // Only clear loading state if this was the last fetch | ||
| if (prNumber === currentFetchPRNumberRef.current) { | ||
| setIsLoadingPRDetails(false); | ||
| } | ||
| }); | ||
|
|
||
| if (prNumber && projectId) { | ||
| // Fetch full PR details including files | ||
| setIsLoadingPRDetails(true); | ||
| window.electronAPI.github.getPR(projectId, prNumber) | ||
| .then(prDetails => { | ||
| if (prDetails) { | ||
| setSelectedPRDetails(prDetails); | ||
| } | ||
| }) | ||
| .catch(err => { | ||
| console.warn(`Failed to fetch PR details for #${prNumber}:`, err); | ||
| }) | ||
| .finally(() => { | ||
| setIsLoadingPRDetails(false); | ||
| }); | ||
|
|
||
| // Load existing review from disk if not already in store | ||
| const existingState = getPRReviewState(projectId, prNumber); | ||
| // Only fetch from disk if we don't have a result in the store | ||
| if (!existingState?.result) { | ||
| window.electronAPI.github.getPRReview(projectId, prNumber).then(result => { | ||
| if (result) { | ||
| // Update store with the loaded result | ||
| // Preserve newCommitsCheck when loading existing review from disk | ||
| usePRReviewStore.getState().setPRReviewResult(projectId, result, { preserveNewCommitsCheck: true }); | ||
|
|
||
| // Always check for new commits when selecting a reviewed PR | ||
| // This ensures fresh data even if we have a cached check from earlier in the session | ||
| const reviewedCommitSha = result.reviewedCommitSha || (result as any).reviewed_commit_sha; | ||
| if (reviewedCommitSha) { | ||
| window.electronAPI.github.checkNewCommits(projectId, prNumber).then(newCommitsResult => { | ||
| // Load existing review from disk if not already in store | ||
| const existingState = getPRReviewState(projectId, prNumber); | ||
| // Only fetch from disk if we don't have a result in the store | ||
| if (!existingState?.result) { | ||
| window.electronAPI.github.getPRReview(projectId, prNumber).then((result) => { | ||
| if (result) { | ||
| // Update store with the loaded result | ||
| // Preserve newCommitsCheck when loading existing review from disk | ||
| usePRReviewStore | ||
| .getState() | ||
| .setPRReviewResult(projectId, result, { preserveNewCommitsCheck: true }); | ||
|
|
||
| // Always check for new commits when selecting a reviewed PR | ||
| // This ensures fresh data even if we have a cached check from earlier in the session | ||
| const reviewedCommitSha = result.reviewedCommitSha; | ||
| if (reviewedCommitSha) { | ||
| window.electronAPI.github | ||
| .checkNewCommits(projectId, prNumber) | ||
| .then((newCommitsResult) => { | ||
| setNewCommitsCheckAction(projectId, prNumber, newCommitsResult); | ||
| }) | ||
| .catch((err) => { | ||
| console.warn(`Failed to check new commits for PR #${prNumber}:`, err); | ||
| }); | ||
| } | ||
| } | ||
| }); | ||
| } else if (existingState?.result) { | ||
| // Review already in store - always check for new commits to get fresh status | ||
| const reviewedCommitSha = existingState.result.reviewedCommitSha; | ||
| if (reviewedCommitSha) { | ||
| window.electronAPI.github | ||
| .checkNewCommits(projectId, prNumber) | ||
| .then((newCommitsResult) => { | ||
| setNewCommitsCheckAction(projectId, prNumber, newCommitsResult); | ||
| }).catch(err => { | ||
| }) | ||
| .catch((err) => { | ||
| console.warn(`Failed to check new commits for PR #${prNumber}:`, err); | ||
| }); | ||
| } | ||
| } | ||
| }); | ||
| } else if (existingState?.result) { | ||
| // Review already in store - always check for new commits to get fresh status | ||
| const reviewedCommitSha = existingState.result.reviewedCommitSha || (existingState.result as any).reviewed_commit_sha; | ||
| if (reviewedCommitSha) { | ||
| window.electronAPI.github.checkNewCommits(projectId, prNumber).then(newCommitsResult => { | ||
| setNewCommitsCheckAction(projectId, prNumber, newCommitsResult); | ||
| }).catch(err => { | ||
| console.warn(`Failed to check new commits for PR #${prNumber}:`, err); | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| }, [projectId, getPRReviewState, setNewCommitsCheckAction]); | ||
| }, | ||
| [projectId, getPRReviewState, setNewCommitsCheckAction] | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Race condition handling is correct, but consider extracting duplicate logic.
The race condition prevention using currentFetchPRNumberRef is well-implemented. However, the new commits check logic appears twice (lines 299-310 and 314-325) with identical code.
♻️ Optional: Extract duplicate new commits check logic
[projectId, getPRReviewState, setNewCommitsCheckAction]
);
+
+ // Helper to check new commits for a PR
+ const checkNewCommitsForPR = useCallback(
+ (prNumber: number, reviewedCommitSha: string | undefined) => {
+ if (!projectId || !reviewedCommitSha) return;
+ window.electronAPI.github
+ .checkNewCommits(projectId, prNumber)
+ .then((newCommitsResult) => {
+ setNewCommitsCheckAction(projectId, prNumber, newCommitsResult);
+ })
+ .catch((err) => {
+ console.warn(`Failed to check new commits for PR #${prNumber}:`, err);
+ });
+ },
+ [projectId, setNewCommitsCheckAction]
+ );Then use checkNewCommitsForPR(prNumber, result.reviewedCommitSha) in both places.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts
around lines 249 - 330, The new-commits check block is duplicated in selectPR;
extract it into a helper like checkNewCommitsForPR(prNumber: number,
reviewedCommitSha?: string) that calls
window.electronAPI.github.checkNewCommits(projectId, prNumber) and then
setNewCommitsCheckAction(projectId, prNumber, newCommitsResult) (with
console.warn on error); call this helper in both places where you currently
check reviewedCommitSha (the branch after loading result from disk and the
branch for existingState.result). Ensure the helper is used inside the same
scope where projectId, setNewCommitsCheckAction and window.electronAPI are
available and referenced from selectPR/current module.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
✅ Auto Claude Review - APPROVED
Status: Ready to Merge
Summary: No new commits since last review. Previous findings still apply.
💡 Suggestions (1)
These are non-blocking suggestions for consideration:
🔵 [a97fbbfc35b4] [LOW] [UNRESOLVED] [UNRESOLVED] GitLab MRs hook missing isLoadingMRDetails state for consistency
📁 apps/frontend/src/renderer/components/gitlab-merge-requests/hooks/useGitLabMRs.ts:95
The useGitHubPRs hook now exposes isLoadingPRDetails state for showing loading indicators while fetching full PR details. The parallel useGitLabMRs hook lacks this, creating inconsistent UX between GitHub and GitLab integrations.
Resolution note: useGitLabMRs.ts was not modified in this PR. The hook lacks isLoadingMRDetails state while useGitHubPRs exposes this state.
Resolution note: useGitHubPRs.ts has: isLoadingPRDetails: boolean (line 27), const [isLoadingPRDetails, setIsLoadingPRDetails] = useState(false) (line 71), returned in hook (line 497). useGitLabMRs.ts interface UseGitLabMRsResult (lines 18-48) has no equivalent property, no state variable, and nothing returned.
Suggested fix:
Add isLoadingMRDetails and selectedMRDetails state to useGitLabMRs hook following the same pattern as useGitHubPRs
This automated review found no blocking issues. The PR can be safely merged.
Generated by Auto Claude
StillKnotKnown
deleted the
stillknotknown/acs-173-bug-github-pr-view-missing-files-changed-list-and-change
branch
Upstream changes merged: - PR AndyMik90#824: Fix Kanban state transitions and status flip-flop bug (ACS-51, ACS-55, ACS-71) - PR AndyMik90#822: Fix GitHub PR files changed list (use selectedPR from hook) - PR AndyMik90#805: Add Azure Trusted Signing for Windows builds Breaking changes: - Removed local ResumeOrFreshDialog, StartFreshDialog, GlobalResumeOrFreshDialog (upstream handles stuck tasks differently via phase validation) - task-store API simplified to match upstream Co-Authored-By: Claude Opus 4.5 <[email protected]>
Upstream changes merged: - PR AndyMik90#824: Fix Kanban state transitions and status flip-flop bug - PR AndyMik90#822: Fix GitHub PR files changed list - PR AndyMik90#805: Add Azure Trusted Signing for Windows builds Local customizations preserved: - Delete Task feature with confirmation dialog - Restart App button in Debug settings - TASK_STOP worktree sync fix for status persistence Co-Authored-By: Claude Opus 4.5 <[email protected]>
3 participants
Base Branch
developbranch (required for all feature/fix PRs)main(hotfix only - maintainers)Description
Fixes bug where GitHub PR detail view showed "File list not available" and missing file change count. The root cause was that
GitHubPRs.tsxwas doing its own PR lookup in theprsarray (list-view data without files) instead of using theselectedPRfrom the hook (which includes full details with files).This PR also addresses code consistency issues:
useGitHubPRs.tsto preserve previous PR details during fetch, preventing brief empty state when switching between PRsuseGitLabIssuesanduseGitHubIssueshooks now returnselectedIssue(computed viauseMemo), eliminating manual lookup in components?? 0fallbacks foradditions,deletions,changedFilesto handle edge cases where GitHub API returns nullRelated Issue
Closes #821
Type of Change
Area
Commit Message Format
Follow conventional commits:
<type>: <subject>Types: feat, fix, docs, style, refactor, test
Example:
feat: add user authentication systemChecklist
developbranchCI/Testing Requirements
Screenshots
Feature Toggle
use_feature_nameBreaking Changes
Breaking: No
Details: None - this is a bug fix with no breaking changes
Summary by CodeRabbit
New Features
Refactor
Style
✏️ Tip: You can customize this high-level summary in your review settings.