Add support for self-hosted GitHub Enterprise and GitLab instances#12
Draft
facundofarias wants to merge 3 commits into
Draft
Add support for self-hosted GitHub Enterprise and GitLab instances#12facundofarias wants to merge 3 commits into
facundofarias wants to merge 3 commits into
Conversation
Backward compatible: existing accounts default to canonical hosts (api.github.com, gitlab.com). The new optional instanceUrl on PlatformAccount routes API calls to the user's instance instead. Setup adds a "Self-hosted instance?" toggle (GitHub and GitLab only; Bitbucket Data Center is deferred). When enabled, the user enters an HTTPS instance URL; the extension requests runtime host permission via chrome.permissions.request before validating the token. Settings shows the host as a subtitle next to @username for self-hosted accounts. Phase 1+2 of the plan in docs/plans/self-hosted-instances.md; Bitbucket DC (different API, ~5-10 days) deferred pending demand. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds support for self-hosted Git providers by introducing an optional ChangesSelf-Hosted Instance Support
Sequence DiagramsequenceDiagram
actor User
participant Setup as Setup Component
participant Utils as Instance Utilities
participant Permissions as Runtime Permissions
participant API as API Client
participant Storage as Account Storage
User->>Setup: Enable "Self-hosted" and enter instance URL
Setup->>Utils: normalizeInstanceUrl(input, platform)
Utils-->>Setup: normalized URL / null
alt invalid URL
Setup-->>User: show validation error
else valid URL
Setup->>Permissions: requestInstanceHostPermission(normalized)
Permissions-->>Setup: granted / denied
alt denied
Setup-->>User: show permission error, block connect
else granted
User->>Setup: click Connect
Setup->>API: getAuthenticatedUser(token, instanceUrl)
API-->>Setup: user info
Setup->>Storage: save account (with instanceUrl)
Storage-->>Setup: stored
Setup-->>User: connection complete
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 2/5 reviews remaining, refill in 24 minutes and 16 seconds. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ba0290ebb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/api/github.ts (1)
617-659:⚠️ Potential issue | 🟠 MajorAvoid silently masking GraphQL failures with zero defaults
The catch block at lines 657-659 returns zero values for all fields on any GraphQL error, with no fallback mechanism or error indication. This silently masks failures (auth issues, GHES field/version mismatches) and makes it impossible for consumers to distinguish actual zero unresolved comments from a failed fetch, under-reporting review risk.
Since the comment at lines 594-595 confirms REST API cannot reliably expose
isResolvedormergeable, consider either:
- Adding explicit error logging/handling so failures are detectable
- Implementing a partial fallback (e.g., REST-based comment count approximation for unresolved threads)
- Returning a nullable or error-state result rather than defaulting to zeros
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/api/github.ts` around lines 617 - 659, The catch currently swallows GraphQL errors and returns zeroed fields (unresolvedCommentCount, hasConflicts, additions, deletions) making failures indistinguishable from real zeros; update the catch to surface the failure by (1) logging the caught error (include the error object) when ghGraphQL fails, and (2) change the returned shape to indicate an error state instead of zeros — e.g. return nullable fields or an explicit error flag/message alongside unresolvedCommentCount/unresolvedCommentAuthors/hasConflicts (references: ghGraphQL, GraphQLPullRequestData, unresolvedCommentCount, unresolvedCommentAuthors, hasConflicts) so callers can detect and handle GraphQL failures.
🧹 Nitpick comments (1)
src/popup/pages/Setup.tsx (1)
202-210: ⚡ Quick winExpose expanded/collapsed state on the account panel trigger
The collapsible trigger button should expose
aria-expanded(and ideallyaria-controls) so assistive tech can track panel state.As per coding guidelines, "Use ARIA labels, roles (tab, switch, checkbox), aria-live regions, aria-expanded on collapsible sections, and hide decorative icons from screen readers."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/popup/pages/Setup.tsx` around lines 202 - 210, The button trigger for the account panel does not expose ARIA state; update the button element (the one with onClick that calls setConnectingPlatform and resetForm) to include aria-expanded={isExpanded} and an aria-controls attribute referencing the panel id (e.g. aria-controls={`account-panel-${cfg.platform}`}); also ensure the collapsible panel element uses the matching id (id={`account-panel-${cfg.platform}`}) so assistive tech can associate the trigger with the panel and track expanded/collapsed state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/popup/pages/Setup.tsx`:
- Around line 119-127: The normalized self-hosted URL can include API path
segments (e.g., /api/v4) which later get duplicated by resolveBaseUrl called
from github.getAuthenticatedUser() or gitlab.getAuthenticatedUser(); update the
Setup flow to strip any trailing API suffixes before using the normalized value:
after calling normalizeInstanceUrl(instanceUrl) (inside the selfHosted branch)
remove trailing segments like /api, /api/v3, /api/v4 (case-insensitive, with or
without trailing slash) and then assign that cleaned value to normalizedInstance
so the subsequent calls to github.getAuthenticatedUser() /
gitlab.getAuthenticatedUser() receive a base host URL without API paths.
In `@src/shared/api/github.ts`:
- Around line 262-263: The delete request is using the raw branch name in the
path which breaks for branch names with slashes; update the URL construction to
URL-encode the branch segment (use encodeURIComponent(branch)) when building
`${baseUrl}/repos/${repoFullName}/git/refs/heads/${branch}` so the fetch call in
this module (the constant `res` creation) uses the encoded branch; keep other
path segments as-is and mirror the approach used in gitlab.ts for path parameter
encoding.
---
Outside diff comments:
In `@src/shared/api/github.ts`:
- Around line 617-659: The catch currently swallows GraphQL errors and returns
zeroed fields (unresolvedCommentCount, hasConflicts, additions, deletions)
making failures indistinguishable from real zeros; update the catch to surface
the failure by (1) logging the caught error (include the error object) when
ghGraphQL fails, and (2) change the returned shape to indicate an error state
instead of zeros — e.g. return nullable fields or an explicit error flag/message
alongside unresolvedCommentCount/unresolvedCommentAuthors/hasConflicts
(references: ghGraphQL, GraphQLPullRequestData, unresolvedCommentCount,
unresolvedCommentAuthors, hasConflicts) so callers can detect and handle GraphQL
failures.
---
Nitpick comments:
In `@src/popup/pages/Setup.tsx`:
- Around line 202-210: The button trigger for the account panel does not expose
ARIA state; update the button element (the one with onClick that calls
setConnectingPlatform and resetForm) to include aria-expanded={isExpanded} and
an aria-controls attribute referencing the panel id (e.g.
aria-controls={`account-panel-${cfg.platform}`}); also ensure the collapsible
panel element uses the matching id (id={`account-panel-${cfg.platform}`}) so
assistive tech can associate the trigger with the panel and track
expanded/collapsed state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78f50bd9-1380-4320-930a-d616f3d14960
📒 Files selected for processing (10)
docs/plans/self-hosted-instances.mdmanifest.jsonsrc/background/service-worker.tssrc/popup/pages/Repos.tsxsrc/popup/pages/Settings.tsxsrc/popup/pages/Setup.tsxsrc/shared/api/github.tssrc/shared/api/gitlab.tssrc/shared/instanceUrl.tssrc/shared/types.ts
Covers the new instanceUrl helper module (normalization, canonical-default fallback, self-hosted detection, host display) and verifies that the GitHub and GitLab API clients route requests to the configured instance URL while preserving the canonical default when none is set. Also asserts pagination links from a GHES /api/v3 base are followed correctly back to the same GHES host. 31 new tests; full suite is 113 passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two fixes for the self-hosted-instance flow surfaced in PR review: - normalizeInstanceUrl now takes an optional platform argument. When provided, it strips well-known API path suffixes (/api/v3, /api/graphql for GitHub; /api/v4 for GitLab) so users who accidentally paste an API endpoint don't end up with a double-appended path (e.g., /api/v4/api/v4). It also returns null when the cleaned URL matches the canonical service URL — Setup uses that to surface a clearer "untoggle Self-hosted to use the public service" error instead of persisting bogus state. - resolveBaseUrl in github.ts is now defensive: if a caller passes the canonical github.com instance URL (legacy data or any path that bypassed Setup), it routes to the cloud REST/GraphQL endpoints rather than the broken https://github.com/api/v3. Adds 9 tests covering the new normalization paths and the defensive canonical-routing in the GitHub client. Full suite: 122 passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1.
Summary
Adds opt-in support for GitHub Enterprise Server and GitLab self-managed instances. Bitbucket Data Center is deferred (different API entirely; tracked in #1 for a follow-up if demand surfaces).
Backward compatible: existing accounts have no
instanceUrlfield and continue to hit the canonical hosts. Users ongithub.com/gitlab.comsee no UI change unless they toggle the new option.What changed
instanceUrlfield onPlatformAccount. Absent ⇒ canonical service.instanceUrlparameter, parameterizing the base URL (/api/v3for GHES,/api/v4for GitLab).chrome.permissions.request) before the first API call to a self-hosted host. Manifest declaresoptional_host_permissions: ["https://*/*"](Chrome) andoptional_permissions(Firefox).@user · gitlab.example.com) for self-hosted accounts.docs/plans/self-hosted-instances.md.Why no Bitbucket Data Center yet
Bitbucket DC uses a different REST API path (
/rest/api/1.0/...), different auth (HTTP access tokens vs. Cloud'semail:tokenBasic), and entirely different data shapes. Implementing it would effectively be a new platform client (~5-10 days). Deferring until there's measurable demand. Toggle is hidden for Bitbucket so users aren't misled.Status: needs real-instance testing
I implemented this without access to a real GitHub Enterprise Server or GitLab self-managed instance, so the code compiles and unit tests pass but end-to-end behavior against a real self-hosted server has not been verified. Help wanted before merging.
Test plan
npm run typecheck) — passesnpm run lint) — passesnpx vitest run) — 82/82 passingnpm run build,npm run build:firefox) — both succeed; manifests emit correctoptional_host_permissions/optional_permissionsgithub.com,gitlab.com) accounts keep working — no signature changes that break absentinstanceUrlinstanceUrlcorrectlyNotes for review
getNextPagePath) now derives the expected origin from the configured base URL, so GHESLinkheaders pointing atghes.example.com/api/v3/...are followed correctly. Existing test still covers the canonical path.https://*/*) is only requested when a user explicitly enters a self-hosted URL — users who never toggle self-hosted never see a permission prompt. Worth flagging in the Chrome Web Store re-review notes.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores
Tests