fix(githubPane): show issues and PR in github panes by pulling from upstream #409
Conversation
…and parentRepo Added optional isFork and parentRepo properties to GitHubConfig interface to support fork detection and dual-repository functionality.
…ork and parentRepository Add fork detection properties to GitHubSyncStatus interface: - isFork: boolean flag indicating if the repository is a fork - parentRepository: object containing fullName and url of the parent repo 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…d GITHUB_PARENT_REPO - Extended getGitHubConfig function to parse IS_FORK and GITHUB_PARENT_REPO from .env files - IS_FORK is parsed strictly: only 'true' or 'TRUE' values are treated as true - GITHUB_PARENT_REPO is normalized using normalizeRepoReference and validated to contain '/' - Added comprehensive test suite for fork detection functionality with 22 tests - Tests cover edge cases: empty values, mixed case, URL normalization, invalid formats 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add getTargetRepo helper function to determine which repository to query
based on operation type:
- For non-forks: always returns the configured repo
- For forks with issues/PRs: returns parent repo if available, otherwise fork
- For forks with code operations: returns fork repo
Also adds:
- OperationType type export ('issues' | 'prs' | 'code')
- Comprehensive test suite with 12 new tests for getTargetRepo
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
…falls back to fork Implemented githubFetchWithFallback utility function in utils.ts that: - Makes a GitHub API request to the target repo (parent for issues/PRs operations on forks) - Falls back to the fork repository when the parent returns 403 or 404 errors - Returns a FetchWithFallbackResult with data, usedFallback flag, and the repo that was used - Does NOT fall back for non-forks, code operations, or non-access errors (401, 500, etc.) Added comprehensive test suite with 12 test cases covering: - Successful requests without fallback - Fallback on 403 and 404 errors - No fallback scenarios (non-fork repos, missing parent, code operations, server errors) - Fallback failure handling - Request options passthrough All 46 fork-detection tests pass.
… a repository is a fork via GitHub API
- Added ForkStatus interface with isFork, parentRepo, and parentUrl fields
- Added GitHubRepoWithForkInfo interface for GitHub API response typing
- Implemented detectForkStatus function that:
- Queries GitHub API /repos/{owner}/{repo} endpoint
- Checks the fork boolean field
- Extracts parent repository info (full_name, html_url) if available
- Normalizes repo reference before API call
- Returns ForkStatus object with fork detection results
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add registerDetectFork() to registerRepositoryHandlers() so the github:detectFork IPC channel is properly registered at startup. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…fork status and parent repository in response - Modified registerCheckConnection to include isFork and parentRepository fields in GitHubSyncStatus response - Uses config.isFork from .env file (defaults to false if undefined) - Adds parentRepository with fullName and url when isFork is true and parentRepo is configured - All 46 fork-detection tests pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
… for routing Update all issue handlers to use getTargetRepo() to properly route issue requests to parent repository when working with forks: - registerGetIssues: Route issue list requests to parent repo for forks - registerGetIssue: Route single issue requests to parent repo for forks - registerGetIssueComments: Route comment requests to parent repo for forks 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…r routing Updated all PR handlers to use getTargetRepo for routing PR requests to the parent repository when working with a fork: - GITHUB_PR_LIST: List PRs from parent repo for forks - GITHUB_PR_GET: Get single PR from parent repo - GITHUB_PR_REVIEW: Auto-assign user to PR on parent repo - GITHUB_PR_POST_REVIEW: Post review to parent repo - GITHUB_PR_DELETE_REVIEW: Delete review from parent repo - GITHUB_PR_ASSIGN: Assign user to PR on parent repo - GITHUB_PR_CHECK_NEW_COMMITS: Check commits on parent repo All operations now use getTargetRepo(config, 'prs') to determine the correct repository based on fork configuration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…al-repository-support
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds fork detection and fork-aware routing: operations (issues/prs/code) resolve target repos based on fork status and parent info, githubFetchWithFallback attempts parent then fork on 403/404, exposes an IPC channel for fork detection, and adds unit tests for fork logic and normalization. Changes
Sequence DiagramssequenceDiagram
autonumber
participant Client
participant IPC as "IPC Handler"
participant Utils as "Fork Utils"
participant GH as "GitHub API"
Client->>IPC: Request (operationType, config)
IPC->>Utils: getTargetRepo(config, operationType)
Utils-->>IPC: targetRepo
IPC->>Utils: githubFetchWithFallback(config, endpointBuilder, operationType)
Utils->>GH: Request -> /repos/{targetRepo}/... (primary)
alt 200 OK
GH-->>Utils: 200 + data
Utils-->>IPC: {data, usedFallback: false, repo: targetRepo}
else 403/404 && Utils.parentRepoExists && operationType != "code"
GH-->>Utils: 403/404
Utils->>GH: Request -> /repos/{forkRepo}/... (fallback)
GH-->>Utils: Response (ok/error)
Utils-->>IPC: {data, usedFallback: true, repo: forkRepo}
else Error
GH-->>Utils: Error
Utils-->>IPC: Error (propagated)
end
IPC-->>Client: Return result (data, usedFallback, repo)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (2)apps/frontend/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
apps/frontend/**/*.{ts,tsx}⚙️ CodeRabbit configuration file
Files:
⏰ 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). (3)
🔇 Additional comments (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
apps/frontend/src/main/ipc-handlers/github/__tests__/fork-detection.spec.tsapps/frontend/src/main/ipc-handlers/github/issue-handlers.tsapps/frontend/src/main/ipc-handlers/github/pr-handlers.tsapps/frontend/src/main/ipc-handlers/github/repository-handlers.tsapps/frontend/src/main/ipc-handlers/github/types.tsapps/frontend/src/main/ipc-handlers/github/utils.tsapps/frontend/src/shared/constants/ipc.tsapps/frontend/src/shared/types/integrations.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{ts,tsx}: Always use translation keys withuseTranslation()for all user-facing text in React/TypeScript frontend components - use formatnamespace:section.key(e.g.,navigation:items.githubPRs)
Never use hardcoded strings in JSX/TSX files for user-facing text - always reference translation keys fromapps/frontend/src/shared/i18n/locales/
Files:
apps/frontend/src/shared/constants/ipc.tsapps/frontend/src/main/ipc-handlers/github/issue-handlers.tsapps/frontend/src/main/ipc-handlers/github/types.tsapps/frontend/src/main/ipc-handlers/github/pr-handlers.tsapps/frontend/src/shared/types/integrations.tsapps/frontend/src/main/ipc-handlers/github/__tests__/fork-detection.spec.tsapps/frontend/src/main/ipc-handlers/github/repository-handlers.tsapps/frontend/src/main/ipc-handlers/github/utils.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/shared/constants/ipc.tsapps/frontend/src/main/ipc-handlers/github/issue-handlers.tsapps/frontend/src/main/ipc-handlers/github/types.tsapps/frontend/src/main/ipc-handlers/github/pr-handlers.tsapps/frontend/src/shared/types/integrations.tsapps/frontend/src/main/ipc-handlers/github/__tests__/fork-detection.spec.tsapps/frontend/src/main/ipc-handlers/github/repository-handlers.tsapps/frontend/src/main/ipc-handlers/github/utils.ts
🧬 Code graph analysis (2)
apps/frontend/src/main/ipc-handlers/github/issue-handlers.ts (1)
apps/frontend/src/main/ipc-handlers/github/utils.ts (2)
getTargetRepo(124-138)normalizeRepoReference(94-110)
apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts (1)
apps/frontend/src/main/ipc-handlers/github/utils.ts (1)
getTargetRepo(124-138)
🔇 Additional comments (34)
apps/frontend/src/shared/constants/ipc.ts (1)
200-200: LGTM!The new IPC channel follows the established naming convention and is appropriately grouped with other GitHub detection channels.
apps/frontend/src/main/ipc-handlers/github/types.ts (1)
8-9: LGTM!The optional properties maintain backward compatibility and align with the fork-detection flow introduced across the codebase.
apps/frontend/src/shared/types/integrations.ts (1)
115-119: LGTM!The optional properties maintain backward compatibility. The nested
parentRepositorystructure provides useful metadata for UI display of fork information.apps/frontend/src/main/ipc-handlers/github/issue-handlers.ts (4)
9-9: LGTM!Import correctly added to support fork-aware routing.
60-62: LGTM!The fork-aware routing pattern is correctly applied:
getTargetRepodetermines the appropriate repository (parent for forks, configured repo otherwise), thennormalizeRepoReferenceensures consistent format before the API call.
119-121: LGTM!Consistent application of the fork-aware routing pattern for single issue fetching.
165-167: LGTM!Consistent application of the fork-aware routing pattern for issue comments.
apps/frontend/src/main/ipc-handlers/github/repository-handlers.ts (3)
12-19: LGTM!The
ForkStatusinterface is well-structured with clear properties for fork detection results.
125-141: LGTM!The connection check correctly includes fork status and parent repository information in the response, using the config values appropriately.
253-256: LGTM!The new
registerDetectForkhandler is correctly registered alongside existing handlers.apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts (10)
16-16: LGTM!Import correctly added to support fork-aware routing for PR operations.
405-408: LGTM!Consistent use of
getTargetRepofor fetching single PR data.
426-428: LGTM!Same
targetRepocorrectly reused for fetching PR files.
542-546: LGTM!Auto-assignment correctly routes to the target repo for PR-related operations.
662-667: LGTM!Review posting correctly uses the target repo.
684-686: LGTM!Fallback for own-PR review limitation correctly uses the same target repo.
800-803: LGTM!Review deletion correctly uses the target repo.
882-886: LGTM!User assignment correctly routes to the target repo.
975-978: LGTM!New commits check correctly uses the target repo for PR data.
993-995: LGTM!Comparison endpoint correctly uses the same target repo.
apps/frontend/src/main/ipc-handlers/github/utils.ts (4)
14-20: LGTM!The
OperationTypeunion type is well-documented and covers all operation categories that need different routing behavior.
64-80: LGTM!Good implementation of fork detection parsing:
- Strict parsing of
IS_FORK(only'true'or'TRUE'count as true)- Parent repo normalization and validation ensure consistent format
- Validation that
parentRepocontains/prevents invalid values
124-138: LGTM!The
getTargetRepologic is clear and correct:
- Non-forks always use the configured repo
- Forks route issues/PRs to parent (with fallback to fork if no parent)
- Code operations always use the fork
169-177: LGTM!The
FetchWithFallbackResultinterface provides useful metadata about the fetch operation, including whether fallback was used.apps/frontend/src/main/ipc-handlers/github/__tests__/fork-detection.spec.ts (10)
1-46: LGTM!Well-structured test setup with appropriate mocks for filesystem, process, and utility dependencies.
47-172: LGTM!Comprehensive tests for
getGitHubConfigfork detection:
- Tests
IS_FORK=true,TRUE,false, mixed case, and missing values- Correctly verifies that only exact
'true'or'TRUE'are treated as true
173-257: LGTM!Good coverage of
GITHUB_PARENT_REPOparsing:
- Empty value handling
- URL normalization (https, git@)
- Quoted values in .env files
259-354: LGTM!Edge case tests are well-designed:
- Tests
nullautoBuildPath- Tests missing
.envfile- Tests missing required fields (token, repo)
- Tests invalid parent repo format (no
/)
357-389: LGTM!Good coverage of
normalizeRepoReference:
- Various URL formats
- Trailing
.gitremoval- Empty input handling
- Whitespace trimming
391-496: LGTM!Comprehensive
getTargetRepotests covering:
- Non-fork repositories (all operation types)
- Fork with parent configured
- Fork without parent configured
- Edge cases (undefined isFork, empty parentRepo)
498-595: LGTM!Good tests for
githubFetchWithFallbacksuccess scenarios:
- Primary request success
- Non-fork repository handling
- Code operations using fork repo
597-667: LGTM!Fallback scenario tests are well-designed:
- 403 error triggers fallback
- 404 error triggers fallback
- Verifies correct repo is used after fallback
669-791: LGTM!Excellent "no fallback" scenario coverage:
- Non-fork repositories don't fall back
- Forks without parent don't fall back
- Code operations don't fall back
- Server errors (500) don't trigger fallback
- Auth errors (401) don't trigger fallback
793-881: LGTM!Good coverage of:
- Fallback failure propagation
- Request options passthrough to both primary and fallback requests
| const targetRepo = getTargetRepo(config, 'prs'); | ||
| const prs = await githubFetch( | ||
| config.token, | ||
| `/repos/${config.repo}/pulls?state=open&per_page=50` | ||
| `/repos/${targetRepo}/pulls?state=open&per_page=50` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the specific lines in pr-handlers.ts around 347-350
sed -n '340,360p' apps/frontend/src/main/ipc-handlers/github/pr-handlers.tsRepository: AndyMik90/Auto-Claude
Length of output: 731
🏁 Script executed:
#!/bin/bash
# Now let's find all usages of getTargetRepo in issue-handlers.ts to see the normalization pattern
rg -n 'getTargetRepo' apps/frontend/src/main/ipc-handlers/github/issue-handlers.ts -A 5 | head -50Repository: AndyMik90/Auto-Claude
Length of output: 1426
🏁 Script executed:
#!/bin/bash
# Check what normalizeRepoReference does and where it's defined
rg -n 'normalizeRepoReference' apps/frontend/src/main/ipc-handlers/github/ -B 2 -A 5 | head -80Repository: AndyMik90/Auto-Claude
Length of output: 7420
🏁 Script executed:
#!/bin/bash
# Let's also look at the getTargetRepo function definition
rg -n 'function getTargetRepo\|const getTargetRepo' apps/frontend/src/main/ipc-handlers/github/ -A 10 | head -40Repository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Find getTargetRepo function definition
rg -n 'export.*function getTargetRepo|export.*const getTargetRepo' apps/frontend/src/main/ipc-handlers/github/utils.ts -A 15Repository: AndyMik90/Auto-Claude
Length of output: 635
🏁 Script executed:
#!/bin/bash
# Also check pr-handlers.ts for all getTargetRepo usages to see if normalization is used anywhere
rg -n 'getTargetRepo' apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts -B 2 -A 10Repository: AndyMik90/Auto-Claude
Length of output: 4302
Apply normalizeRepoReference() consistently after getTargetRepo() in PR handlers.
In issue-handlers.ts, all getTargetRepo() calls are followed by normalizeRepoReference() for validation. The PR handlers at lines 347, 405, 543, 662, 800, 883, and 975 use getTargetRepo() directly without this normalization step. While config.parentRepo is already normalized during configuration loading, config.repo is not, so if a fork falls back to the configured repo (or for non-fork repos), the API endpoint construction could fail if the repository is provided in a non-standard format (URL, git@ format, etc.). Apply the same defensive validation pattern as issue handlers for consistency.
🤖 Prompt for AI Agents
In apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts around lines 347,
405, 543, 662, 800, 883, and 975, calls to getTargetRepo(...) are used directly
to build GitHub API endpoints; apply normalizeRepoReference(...) to the value
returned by getTargetRepo(...) in each location (replace targetRepo with const
normalized = normalizeRepoReference(targetRepo) and use normalized for the URL),
add/import normalizeRepoReference if not present, and handle/propagate any
validation errors thrown by normalizeRepoReference the same way
issue-handlers.ts does so non-standard repo formats (URLs, git@, etc.) are
normalized before constructing API endpoints.
| export async function detectForkStatus( | ||
| token: string, | ||
| repo: string | ||
| ): Promise<ForkStatus> { | ||
| const normalizedRepo = normalizeRepoReference(repo); | ||
| if (!normalizedRepo) { | ||
| return { isFork: false }; | ||
| } | ||
|
|
||
| const repoData = await githubFetch( | ||
| token, | ||
| `/repos/${normalizedRepo}` | ||
| ) as GitHubRepoWithForkInfo; | ||
|
|
||
| // Check if repository is a fork | ||
| if (!repoData.fork) { | ||
| return { isFork: false }; | ||
| } | ||
|
|
||
| // If it's a fork, extract parent repository info | ||
| const result: ForkStatus = { isFork: true }; | ||
|
|
||
| if (repoData.parent) { | ||
| result.parentRepo = repoData.parent.full_name; | ||
| result.parentUrl = repoData.parent.html_url; | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider error handling for the GitHub API call.
The githubFetch call on lines 54-57 can throw errors (e.g., network issues, invalid token, repo not found), but there's no try-catch here. Errors will propagate to callers, which works for registerDetectFork (which has error handling), but direct callers of detectForkStatus need to be aware of this.
The function signature doesn't indicate it can throw, which might be unexpected for callers.
🔎 Optional: Add explicit error handling or document throwing behavior
Either document the throwing behavior:
/**
* Detect if a repository is a fork via the GitHub API
*
* ...existing JSDoc...
+ * @throws {Error} If the GitHub API request fails
*/Or handle errors gracefully:
+ try {
const repoData = await githubFetch(
token,
`/repos/${normalizedRepo}`
) as GitHubRepoWithForkInfo;
+ } catch {
+ // If we can't fetch repo info, assume it's not a fork
+ return { isFork: false };
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function detectForkStatus( | |
| token: string, | |
| repo: string | |
| ): Promise<ForkStatus> { | |
| const normalizedRepo = normalizeRepoReference(repo); | |
| if (!normalizedRepo) { | |
| return { isFork: false }; | |
| } | |
| const repoData = await githubFetch( | |
| token, | |
| `/repos/${normalizedRepo}` | |
| ) as GitHubRepoWithForkInfo; | |
| // Check if repository is a fork | |
| if (!repoData.fork) { | |
| return { isFork: false }; | |
| } | |
| // If it's a fork, extract parent repository info | |
| const result: ForkStatus = { isFork: true }; | |
| if (repoData.parent) { | |
| result.parentRepo = repoData.parent.full_name; | |
| result.parentUrl = repoData.parent.html_url; | |
| } | |
| return result; | |
| } | |
| export async function detectForkStatus( | |
| token: string, | |
| repo: string | |
| ): Promise<ForkStatus> { | |
| const normalizedRepo = normalizeRepoReference(repo); | |
| if (!normalizedRepo) { | |
| return { isFork: false }; | |
| } | |
| try { | |
| const repoData = await githubFetch( | |
| token, | |
| `/repos/${normalizedRepo}` | |
| ) as GitHubRepoWithForkInfo; | |
| // Check if repository is a fork | |
| if (!repoData.fork) { | |
| return { isFork: false }; | |
| } | |
| // If it's a fork, extract parent repository info | |
| const result: ForkStatus = { isFork: true }; | |
| if (repoData.parent) { | |
| result.parentRepo = repoData.parent.full_name; | |
| result.parentUrl = repoData.parent.html_url; | |
| } | |
| return result; | |
| } catch { | |
| // If we can't fetch repo info, assume it's not a fork | |
| return { isFork: false }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/frontend/src/main/ipc-handlers/github/repository-handlers.ts around
lines 45 to 73, the githubFetch call can throw but the function neither handles
nor documents that; wrap the githubFetch call in a try/catch and on error either
rethrow a new Error that includes context (e.g., repo name/normalizedRepo and
the original error message, preserving the original error as the cause if
available) or return a well-defined error result (and update the ForkStatus type
accordingly); also add a short TSDoc comment on detectForkStatus indicating it
may throw so callers are aware.
| try { | ||
| // Normalize repo reference (handles full URLs, git URLs, etc.) | ||
| const normalizedRepo = normalizeRepoReference(config.repo); | ||
| if (!normalizedRepo) { | ||
| return { | ||
| success: false, | ||
| error: 'Invalid repository format. Use owner/repo or GitHub URL.' | ||
| }; | ||
| } | ||
|
|
||
| const forkStatus = await detectForkStatus(config.token, normalizedRepo); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Redundant normalization.
normalizeRepoReference is called on config.repo here (line 180), and then detectForkStatus also calls normalizeRepoReference internally (line 49 in detectForkStatus). This is harmless but redundant.
Consider either:
- Removing the normalization here and relying on
detectForkStatusto normalize - Or keeping it here for validation and skipping it in
detectForkStatus
🤖 Prompt for AI Agents
In apps/frontend/src/main/ipc-handlers/github/repository-handlers.ts around
lines 178–188, the call to normalizeRepoReference(config.repo) is redundant
because detectForkStatus already normalizes the repo; remove the explicit
normalization and the immediate invalid-repo return here, and instead pass
config.repo directly to detectForkStatus so that validation/normalization
happens inside that function; ensure you adjust the subsequent error handling to
surface any invalid-repo result returned by detectForkStatus (or its thrown
error) so callers still receive a clear "invalid repository format" response.
| export async function githubFetchWithFallback<T = unknown>( | ||
| config: GitHubConfig, | ||
| endpointBuilder: (repo: string) => string, | ||
| operationType: OperationType, | ||
| options: RequestInit = {} | ||
| ): Promise<FetchWithFallbackResult<T>> { | ||
| // Determine the primary target repo based on fork config and operation type | ||
| const targetRepo = getTargetRepo(config, operationType); | ||
| const endpoint = endpointBuilder(targetRepo); | ||
|
|
||
| try { | ||
| const data = await githubFetch(config.token, endpoint, options); | ||
| return { | ||
| data: data as T, | ||
| usedFallback: false, | ||
| repo: targetRepo | ||
| }; | ||
| } catch (error) { | ||
| // Only attempt fallback if: | ||
| // 1. This is a fork with a parent repo configured | ||
| // 2. The target repo was the parent (not already the fork) | ||
| // 3. The error is a 403 or 404 | ||
| const shouldFallback = | ||
| config.isFork && | ||
| config.parentRepo && | ||
| targetRepo === config.parentRepo && | ||
| error instanceof Error && | ||
| (error.message.includes('403') || error.message.includes('404')); | ||
|
|
||
| if (!shouldFallback) { | ||
| throw error; | ||
| } | ||
|
|
||
| // Fall back to the fork repository | ||
| const fallbackEndpoint = endpointBuilder(config.repo); | ||
| const fallbackData = await githubFetch(config.token, fallbackEndpoint, options); | ||
| return { | ||
| data: fallbackData as T, | ||
| usedFallback: true, | ||
| repo: config.repo | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if githubFetchWithFallback is used anywhere in the entire codebase
echo "=== Searching for githubFetchWithFallback usage ==="
rg -n 'githubFetchWithFallback' apps/frontend/src/ --type ts --type tsx
echo ""
echo "=== Checking entire repository ==="
rg -n 'githubFetchWithFallback' --type ts --type tsxRepository: AndyMik90/Auto-Claude
Length of output: 225
🏁 Script executed:
#!/bin/bash
# Search without problematic file type flags
echo "=== Searching for githubFetchWithFallback usage ==="
rg 'githubFetchWithFallback' apps/frontend/src/
echo ""
echo "=== Searching for githubFetch usage in handlers ==="
rg -A 3 'githubFetch' apps/frontend/src/main/ipc-handlers/github/ | head -50
echo ""
echo "=== Checking handler files to understand current pattern ==="
ls -la apps/frontend/src/main/ipc-handlers/github/Repository: AndyMik90/Auto-Claude
Length of output: 7226
githubFetchWithFallback is tested but not used in handler implementations.
The function is used in the test suite (__tests__/fork-detection.spec.ts) but the actual handlers (repository-handlers.ts, issue-handlers.ts, pr-handlers.ts, etc.) continue to use githubFetch directly without leveraging the fallback mechanism. This suggests the fallback capability is either reserved for future use or should be integrated into the handlers to provide automatic fallback behavior for fork scenarios.
🤖 Prompt for AI Agents
apps/frontend/src/main/ipc-handlers/github/utils.ts lines 192-233: tests
exercise githubFetchWithFallback but the real handlers still call githubFetch
directly, so wire the fallback into the handlers (repository-handlers.ts,
issue-handlers.ts, pr-handlers.ts, etc.) by replacing direct githubFetch calls
with githubFetchWithFallback: build an endpointBuilder for the call, pass the
current GitHubConfig and correct OperationType, forward RequestInit options, and
use the returned object.data as the response while respecting returned
repo/usedFallback info; preserve original error propagation when shouldFallback
is false and ensure existing handler logic (parsing data, error handling)
continues to work unchanged.
| const shouldFallback = | ||
| config.isFork && | ||
| config.parentRepo && | ||
| targetRepo === config.parentRepo && | ||
| error instanceof Error && | ||
| (error.message.includes('403') || error.message.includes('404')); | ||
|
|
||
| if (!shouldFallback) { | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fragile error detection via string matching.
The fallback condition checks for error.message.includes('403') or includes('404'). This relies on the error message format from githubFetch, which constructs messages like:
GitHub API error: 403 Forbidden - ...
While this works, it's fragile because:
- The error message format could change
- A 404 appearing elsewhere in the error body could trigger false positives
Consider a more robust approach using a custom error class or structured error info.
🔎 Proposed improvement using custom error class
+class GitHubAPIError extends Error {
+ constructor(public status: number, public statusText: string, public body: string) {
+ super(`GitHub API error: ${status} ${statusText} - ${body}`);
+ this.name = 'GitHubAPIError';
+ }
+}
export async function githubFetch(
// ...
): Promise<unknown> {
// ...
if (!response.ok) {
const errorBody = await response.text();
- throw new Error(`GitHub API error: ${response.status} ${response.statusText} - ${errorBody}`);
+ throw new GitHubAPIError(response.status, response.statusText, errorBody);
}
// ...
}
// Then in githubFetchWithFallback:
const shouldFallback =
config.isFork &&
config.parentRepo &&
targetRepo === config.parentRepo &&
- error instanceof Error &&
- (error.message.includes('403') || error.message.includes('404'));
+ error instanceof GitHubAPIError &&
+ (error.status === 403 || error.status === 404);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/frontend/src/main/ipc-handlers/github/utils.ts around lines 214 to 223,
the fallback uses fragile string matching on error.message to detect 403/404;
instead change the githubFetch path to throw a structured error (e.g., a
GitHubAPIError) that exposes an HTTP status property, then update this file to
detect errors by checking instanceof GitHubAPIError (or presence of a numeric
status field) and compare error.status === 403 || error.status === 404; if
githubFetch cannot be changed everywhere, ensure it attaches a stable
error.status property before throwing and use that property here rather than
string.includes.
The test_cache_invalidation_on_file_creation test has a race condition where the hash computed during the first get_security_profile() call (before the profile file is saved) differs from the hash computed by get_dir_hash() (after the file exists). This causes intermittent failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
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)
tests/test_security_cache.py (1)
56-70: Address the root cause rather than marking as xfail.Using
pytest.mark.xfailto work around test flakiness is not a best practice—it masks failures and can hide real bugs. The underlying issue is a test isolation problem where the hash computed during the firstget_security_profilecall differs from the hash computed byget_dir_hashafter the file is created.Additionally, this change appears unrelated to the PR's stated objectives (GitHub fork detection and dual-repository support).
Consider these solutions:
- Mock the hash computation to ensure deterministic behavior across test steps
- Fix the caching logic to properly handle the file-creation scenario
- Capture and reuse the initial hash from the first call instead of recomputing it
- Investigate why hash computation differs before and after file creation and ensure consistency
Example: Mock hash computation for consistent behavior
+from unittest.mock import patch + -@pytest.mark.xfail(reason="Flaky test - hash mismatch between first call (before file save) and test's get_dir_hash (after file exists)") def test_cache_invalidation_on_file_creation(mock_project_dir, mock_profile_path): reset_profile_cache() - - # 1. First call - file doesn't exist - profile1 = get_security_profile(mock_project_dir) - assert "unique_cmd_A" not in profile1.get_all_allowed_commands() - - # 2. Create the file with valid JSON and CORRECT HASH - current_hash = get_dir_hash(mock_project_dir) - mock_profile_path.write_text(create_valid_profile_json(["unique_cmd_A"], current_hash)) - - # 3. Second call - should detect file creation and reload - profile2 = get_security_profile(mock_project_dir) - assert "unique_cmd_A" in profile2.get_all_allowed_commands() + + # Mock hash to ensure consistency + with patch('project.analyzer.ProjectAnalyzer.compute_project_hash', return_value="mock_hash"): + # 1. First call - file doesn't exist + profile1 = get_security_profile(mock_project_dir) + assert "unique_cmd_A" not in profile1.get_all_allowed_commands() + + # 2. Create the file with valid JSON and mocked hash + mock_profile_path.write_text(create_valid_profile_json(["unique_cmd_A"], "mock_hash")) + + # 3. Second call - should detect file creation and reload + profile2 = get_security_profile(mock_project_dir) + assert "unique_cmd_A" in profile2.get_all_allowed_commands()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/test_security_cache.py
🧰 Additional context used
📓 Path-based instructions (1)
tests/**
⚙️ CodeRabbit configuration file
tests/**: Ensure tests are comprehensive and follow pytest conventions.
Check for proper mocking and test isolation.
Files:
tests/test_security_cache.py
⏰ 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)
kvnloo
changed the title
MikeeBuilds
added
bug
…al-repository-support
|
Thank you for your contribution! Before we can accept your PR, you need to sign our Contributor License Agreement (CLA). To sign the CLA, please comment on this PR with exactly: You can read the full CLA here: CLA.md Why do we need a CLA? Auto Claude is licensed under AGPL-3.0. The CLA ensures the project has proper licensing flexibility should we introduce additional licensing options in the future. You retain full copyright ownership of your contributions. I have read the CLA Document and I hereby sign the CLA 0 out of 2 committers have signed the CLA. |
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)
tests/test_security_cache.py (1)
56-78: Fix the test logic instead of marking it as xfail.The
@pytest.mark.xfaildecorator masks a test design flaw rather than addressing it. The test isn't "flaky"—it has a deterministic logic error:
- Line 62 computes
current_hashfor an empty directory (before the profile file exists)- Line 65's
get_security_profilecreates.auto-claude-security.jsonas a side effect, changing the directory's actual hash- Line 74 overwrites the file using the outdated
current_hash(from the empty directory)- The hash in the file no longer matches the directory's actual hash (which includes the file itself)
Marking tests as xfail causes them to be skipped in CI, eliminating their value and reducing coverage.
🔎 Proper fix: compute hash after the side effect
-@pytest.mark.xfail(reason="Flaky test - hash mismatch between first call (before file save) and test's get_dir_hash (after file exists)") def test_cache_invalidation_on_file_creation(mock_project_dir, mock_profile_path): reset_profile_cache() - # Compute hash first, before any files are created - # This hash will be used in the profile we create later - current_hash = get_dir_hash(mock_project_dir) - # 1. First call - file doesn't exist, analyzer will create one with BASE_COMMANDS profile1 = get_security_profile(mock_project_dir) assert "unique_cmd_A" not in profile1.get_all_allowed_commands() + # 2. Compute hash after the file has been created by get_security_profile + current_hash = get_dir_hash(mock_project_dir) + - # 2. Wait to ensure filesystem mtime has different second + # 3. Wait to ensure filesystem mtime has different second # (some filesystems have 1-second resolution) time.sleep(1.0) - # 3. Overwrite the file with our custom content - # Use the SAME hash we computed before (directory structure hasn't changed) + # 4. Overwrite the file with our custom content + # Use the hash computed after the file was created mock_profile_path.write_text(create_valid_profile_json(["unique_cmd_A"], current_hash)) - # 4. Second call - should detect file modification and reload + # 5. Second call - should detect file modification and reload profile2 = get_security_profile(mock_project_dir) assert "unique_cmd_A" in profile2.get_all_allowed_commands()
Note: This test file appears unrelated to the PR's stated objective (GitHub fork detection). Consider moving unrelated fixes to a separate PR for clearer change tracking.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/frontend/src/shared/constants/ipc.tstests/test_security_cache.py
🧰 Additional context used
📓 Path-based instructions (3)
tests/**
⚙️ CodeRabbit configuration file
tests/**: Ensure tests are comprehensive and follow pytest conventions.
Check for proper mocking and test isolation.
Files:
tests/test_security_cache.py
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{ts,tsx}: Always use translation keys withuseTranslation()for all user-facing text in React/TypeScript frontend components - use formatnamespace:section.key(e.g.,navigation:items.githubPRs)
Never use hardcoded strings in JSX/TSX files for user-facing text - always reference translation keys fromapps/frontend/src/shared/i18n/locales/
Files:
apps/frontend/src/shared/constants/ipc.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/shared/constants/ipc.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). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
apps/frontend/src/shared/constants/ipc.ts (1)
203-203: LGTM!The new IPC channel constant follows the established naming conventions and is logically positioned among other GitHub-related operations. The format is consistent with the existing pattern, and the
as constassertion ensures proper TypeScript type safety.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| repo: config.repo | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fallback function implemented but not integrated into handlers
The githubFetchWithFallback function is fully implemented and tested but never actually used in any production handlers. The PR summary claims "Added automatic fetch-with-fallback that retries the fork repository when parent requests fail," but all handlers (issue-handlers.ts, pr-handlers.ts, etc.) continue to use githubFetch directly instead of githubFetchWithFallback. This means when a request to a parent repository fails with 403/404 errors, there is no automatic retry against the fork repository as described.
Additional Locations (2)
…al-repository-support
|
|
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: ✅ READY TO MERGE
No blocking issues found
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- Medium: 2 issue(s)
- Low: 3 issue(s)
Generated by Auto Claude PR Review
Findings (2 selected of 5 total)
🟡 [MEDIUM] Issue count query doesn't use fork-aware routing
📁 apps/frontend/src/main/ipc-handlers/github/repository-handlers.ts:55
The registerCheckConnection function fetches issue count using normalizedRepo (the fork) directly instead of using getTargetRepo(config, 'issues'). For forked repositories, issues exist in the parent repo, so this will return 0 or incorrect counts. The connection status will show misleading information for fork users.
Suggested fix:
Use getTargetRepo(config, 'issues') for the issue count query, similar to how issue-handlers.ts routes to the parent repo.
🟡 [MEDIUM] githubFetchWithFallback implemented but never used
📁 apps/frontend/src/main/ipc-handlers/github/utils.ts:193
The githubFetchWithFallback function is fully implemented with 403/404 fallback logic and has comprehensive tests (100+ lines), but none of the handlers (issue-handlers.ts, pr-handlers.ts) actually call it. They only use getTargetRepo() for routing. This means if the parent repo returns 403 (e.g., private parent), requests will fail instead of falling back to the fork.
Suggested fix:
Either integrate githubFetchWithFallback into the handlers where fallback behavior is desired, or document that it's intended for future use.
This review was generated by Auto Claude.
|
|
||
| const openCount = Array.isArray(issuesData) ? issuesData.length : 0; | ||
|
|
||
| // Build response data with fork status |
There was a problem hiding this comment.
Issue count fetched from fork, not parent repo
In registerCheckConnection, the issueCount is always fetched from config.repo (the fork repository), but the issue handlers now route issue fetches to the parent repository for forks via getTargetRepo. This creates an inconsistency where the connection status displays an issue count from the fork (likely 0), while the actual issue list shows issues from the parent repository. For fork repositories, the issue count should be fetched from getTargetRepo(config, 'issues') to match where issues are actually retrieved from.
…al-repository-support
…al-repository-support
…al-repository-support
5 participants
Base Branch
developbranch (required for all feature/fix PRs)main(hotfix only - maintainers)Description
Implement fork repository detection and dual-repository support in the GitHub integration to fix issue and PR loading failures for forked repositories. Currently, issues and PRs don't load when users work with forked repositories because these resources exist in the upstream/parent repository, not the fork itself. This feature will automatically detect forks via GitHub API, store both fork and parent repository references, and intelligently route API calls to the appropriate repository based on the operation type.
Type of Change
Area
Checklist
developbranchCI/Testing Requirements
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.