Skip to content

Support for github enterprise server(self hosted github) #646

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
May 9, 2025

Conversation

thesujai
Copy link
Contributor

@thesujai thesujai commented Apr 18, 2025

Linked Issue(s)

Fixes #619

Acceptance Criteria fulfillment

Proposed changes (including videos or screenshots)

Further comments

This exactly replicates the custom url implementation for Gitlab

Summary by CodeRabbit

  • New Features

    • Added support for connecting to GitHub Enterprise or custom GitHub API domains throughout the application.
    • Users can now specify a custom domain when configuring GitHub integrations.
  • Enhancements

    • API requests to GitHub dynamically use the appropriate base URL based on user or organization settings.
    • The configuration modal for GitHub now includes an optional input field for a custom domain.
    • Added domain validation for custom GitHub domains to improve input accuracy.
    • Improved error handling and logging for custom domain retrieval and usage.
    • Introduced utility functions for dynamic GitHub API URL construction.
    • Updated Jest test setup to support encoding utilities globally.
  • Bug Fixes

    • Improved handling of custom domains to ensure API calls target the correct endpoints.
  • Tests

    • Added comprehensive tests for GitHub API service initialization with custom domains.
    • Added tests for GitHub URL utility functions and domain validation logic.
    • Added unit tests for domain validation utility function.

Copy link

coderabbitai bot commented Apr 18, 2025

"""

Walkthrough

The changes introduce support for specifying a custom GitHub API domain, enabling integration with GitHub Enterprise or self-hosted GitHub instances. This is achieved by updating backend service constructors and internal logic to accept and propagate an optional domain parameter, which determines the API base URL. Helper functions are added to retrieve and construct the appropriate API URLs based on organization-specific integration metadata. On the frontend, UI components and utility functions are enhanced to allow users to input a custom domain and ensure API requests use the correct endpoint. No changes are made to core business logic or error handling.

Changes

File(s) Change Summary
backend/analytics_server/mhq/exapi/github.py Modified GithubApiService to accept an optional domain parameter, set the base URL accordingly, and added a helper method for URL construction.
backend/analytics_server/mhq/service/code/sync/etl_github_handler.py
.../etl_github_actions_handler.py
Added import of get_custom_github_domain and updated instantiation of GithubApiService to pass the custom domain per organization.
backend/analytics_server/mhq/service/external_integrations_service.py Updated methods to pass an optional custom domain when creating GithubApiService instances.
backend/analytics_server/mhq/utils/github.py Added function get_custom_github_domain to retrieve custom GitHub API domain from organization integration metadata.
backend/analytics_server/tests/exapi/test_github.py Added unit tests for GithubApiService verifying correct base URL handling with default, empty, and custom domains.
web-server/pages/api/internal/[org_id]/utils.ts Introduced helper functions to fetch and construct GitHub API URLs dynamically based on a custom domain from the database; updated API calls to use these helpers.
web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx Enhanced UI to support an optional "Custom Domain" input, updated state management and submission logic to include the custom domain in validation and linking flows.
web-server/src/utils/auth.ts Updated checkGitHubValidity and getMissingPATScopes to accept an optional customDomain parameter and construct API URLs accordingly.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant UI as ConfigureGithubModalBody
    participant AuthUtils as auth.ts
    participant BackendAPI as backend services

    User->>UI: Enter GitHub token and optional custom domain
    UI->>AuthUtils: checkGitHubValidity(token, customDomain)
    AuthUtils->>BackendAPI: Request with baseUrl (customDomain or default)
    BackendAPI-->>AuthUtils: Respond with validity
    AuthUtils-->>UI: Return result
    UI->>BackendAPI: linkProvider(token, customDomain)
    BackendAPI->>BackendAPI: Use custom domain for API calls
    BackendAPI-->>UI: Confirmation
Loading

Assessment against linked issues

Objective Addressed Explanation
Allow changing GitHub server hostname for self-hosted/enterprise instances (#619)
Support specifying the GitHub API endpoint via configuration or user input (#619)
Ensure API requests use the custom hostname when provided (#619)

Poem

A bunny hops with nimble cheer,
Now GitHub’s home is far or near!
Custom domains, we now embrace,
For Enterprise or any place.
With every hop, a hostname new—
This rabbit codes for all of you!
🐇✨
"""

Tip

⚡️ Faster reviews with caching
  • CodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure Review - Disable Cache at either the organization or repository level. If you prefer to disable all data retention across your organization, simply turn off the Data Retention setting under your Organization Settings.

Enjoy the performance boost—your workflow just got faster.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cad13c0 and fffc1bf.

📒 Files selected for processing (2)
  • web-server/pages/api/internal/[org_id]/utils.ts (4 hunks)
  • web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • web-server/pages/api/internal/[org_id]/utils.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx (6)
cli/source/store/index.ts (1)
  • useDispatch (30-30)
web-server/src/utils/fn.ts (1)
  • depFn (1-2)
web-server/src/utils/domainCheck.ts (1)
  • checkDomainWithRegex (1-5)
web-server/src/utils/auth.ts (3)
  • checkGitHubValidity (31-49)
  • getMissingPATScopes (52-72)
  • linkProvider (13-27)
web-server/src/slices/auth.ts (1)
  • fetchCurrentOrg (75-81)
web-server/src/slices/team.ts (1)
  • fetchTeams (136-146)
🔇 Additional comments (7)
web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx (7)

20-20: Using domain validation utility correctly

Good use of importing the checkDomainWithRegex utility to validate custom GitHub Enterprise domains.


29-30: State management for custom domain properly implemented

You've correctly implemented state management for the custom domain input and error handling using useEasyState, following the same pattern as other inputs in this component.

Also applies to: 34-34


43-48: Good error handling pattern for domain validation

The setDomainError callback follows the same pattern as the existing setError function, maintaining consistency in the error handling approach.


59-109: Improved error handling with async/await pattern

Good refactoring of the submission logic to use async/await with try/catch/finally pattern, which improves error handling and readability. The validation of the custom domain is properly implemented.


109-120: Dependencies correctly updated for handleSubmission

The dependency array has been properly updated to include the new customDomain.value and setDomainError dependencies.


178-211: Well-structured UI for custom domain input

The custom domain input UI is well structured with appropriate labels, placeholder, and helper text. The dynamic label that shows "(Optional)" when empty and "Custom Domain" when focused or filled is a good UX design.


212-214: Good error display implementation

Good implementation of error display for the custom domain input. The minimum height ensures consistent layout even when no error is displayed.

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@thesujai thesujai changed the title Supported for github enterprise server(self hosted github) Support for github enterprise server(self hosted github) Apr 18, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

🔭 Outside diff range comments (2)
backend/analytics_server/mhq/service/code/sync/etl_github_handler.py (1)

1-399: 💡 Verification agent

🧩 Analysis chain

Fix Black formatting issues.

The file is flagged by the Black integration pipeline - please run Black to fix the formatting.


🏁 Script executed:

#!/bin/bash
# Run black to check formatting issues
black --check backend/analytics_server/mhq/service/code/sync/etl_github_handler.py

Length of output: 180


To verify formatting, let’s install Black and re-run the check:


🏁 Script executed:

#!/bin/bash
# Install Black formatter and re-check the file
pip install black
black --check backend/analytics_server/mhq/service/code/sync/etl_github_handler.py

Length of output: 1364


Let’s run Black via Python’s module path to ensure it’s found:


🏁 Script executed:

#!/bin/bash
python3 -m black --check backend/analytics_server/mhq/service/code/sync/etl_github_handler.py

Length of output: 225


Please fix Black formatting issues

The file backend/analytics_server/mhq/service/code/sync/etl_github_handler.py is not formatted according to Black (Black reports it “would reformat” this file). Please run Black locally to apply the required changes:

python3 -m black backend/analytics_server/mhq/service/code/sync/etl_github_handler.py

After formatting, commit the updated file.

🧰 Tools
🪛 GitHub Actions: Black Integration

[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.

web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx (1)

103-112: 🛠️ Refactor suggestion

Update dependency array for handleSubmission

The dependency array for handleSubmission is missing customDomain.valueRef which is used in the function.

Add the missing dependency:

}, [
  dispatch,
  enqueueSnackbar,
  isLoading.false,
  isLoading.true,
  onClose,
  orgId,
  setError,
- token.value
+ token.value,
+ customDomain.valueRef
]);
🧹 Nitpick comments (5)
backend/analytics_server/mhq/exapi/github.py (1)

25-32: Fix trailing whitespace and improve formatting.

The GitHub API service constructor has been updated to support custom domains, but there are formatting issues.

-    def __init__(self, access_token: str, domain = "https://api.github.com"):
+    def __init__(self, access_token: str, domain: str = "https://api.github.com"):
     self._token = access_token
     self.base_url = self._get_api_url(domain)
     self._g = Github(
-            self._token, 
+            self._token,
             base_url=self.base_url,
             per_page=PAGE_SIZE
         )

Note the addition of the type hint for domain and removal of trailing whitespace.

web-server/pages/api/internal/[org_id]/utils.ts (1)

324-327: Apply URL sanitization to GraphQL endpoint

Unlike the REST API URL construction which handles potential double slashes, the GraphQL URL construction doesn't have this safeguard.

Add the same safeguard to the GraphQL URL:

const getGitHubGraphQLUrl = async (): Promise<string> => {
  const customDomain = await getGitHubCustomDomain();
-  return customDomain ? `https://${customDomain}/api/graphql` : GITHUB_API_URL;
+  const url = customDomain 
+    ? `https://${customDomain}/api/graphql` 
+    : GITHUB_API_URL;
+  return url.replace(/\/+/g, '/');
};
web-server/src/utils/auth.ts (2)

42-44: Add slash normalization for API URL construction

When building the API URL path, multiple slashes could be introduced, similar to what's handled in the utils.ts file.

Normalize slashes in URL construction:

-  await axios.get(`${baseUrl}/user`, {
+  await axios.get(`${baseUrl}/user`.replace(/\/+/g, '/'), {

54-55: Update function documentation and dependency array

The getMissingPATScopes function signature has changed, but the JSDoc and dependency array in the parent component are not updated.

Add JSDoc to describe the new parameter:

+ /**
+  * Gets the missing PAT scopes for a GitHub token
+  * @param pat - The personal access token to check
+  * @param customDomain - Optional custom domain for GitHub Enterprise
+  * @returns Array of missing scopes
+  */
export const getMissingPATScopes = async (pat: string, customDomain?: string) => {
web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx (1)

74-77: Add documentation for the custom_domain parameter

The code sends custom_domain as part of meta data but there's no documentation about its format requirements.

Add a comment explaining the expected format:

return await linkProvider(token.value, orgId, Integration.GITHUB, {
+ // custom_domain should be a domain name with or without protocol
+ // e.g., "github.mycompany.com" or "https://github.mycompany.com"
  custom_domain: customDomain.valueRef.current
});
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8a545c0 and 2dbf898.

📒 Files selected for processing (7)
  • backend/analytics_server/mhq/exapi/github.py (1 hunks)
  • backend/analytics_server/mhq/service/code/sync/etl_github_handler.py (2 hunks)
  • backend/analytics_server/mhq/service/external_integrations_service.py (1 hunks)
  • backend/analytics_server/mhq/service/workflows/sync/etl_github_actions_handler.py (2 hunks)
  • web-server/pages/api/internal/[org_id]/utils.ts (3 hunks)
  • web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx (6 hunks)
  • web-server/src/utils/auth.ts (3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
web-server/src/utils/auth.ts (1)
web-server/http-server.js (1)
  • axios (5-5)
web-server/pages/api/internal/[org_id]/utils.ts (1)
web-server/src/constants/db.ts (1)
  • Row (557-560)
🪛 GitHub Actions: Black Integration
backend/analytics_server/mhq/service/code/sync/etl_github_handler.py

[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.

backend/analytics_server/mhq/service/workflows/sync/etl_github_actions_handler.py

[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.

backend/analytics_server/mhq/exapi/github.py

[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: All file linting
🔇 Additional comments (14)
backend/analytics_server/mhq/service/external_integrations_service.py (4)

28-29: Consistent custom domain parameter propagation.

The GitHub API service instance is now created with the custom domain parameter, which allows connecting to GitHub Enterprise instances.


36-37: LGTM: GitHub API service initialized with custom domain.

The GitHub API service is properly initialized with the custom domain parameter.


43-44: LGTM: GitHub API service initialized with custom domain.

The GitHub API service is properly initialized with the custom domain parameter.


50-51: LGTM: GitHub API service initialized with custom domain.

The GitHub API service is properly initialized with the custom domain parameter.

backend/analytics_server/mhq/service/code/sync/etl_github_handler.py (1)

392-394: LGTM: GitHub API service initialized with custom domain.

The GitHub API service is properly initialized with both the access token and custom domain.

backend/analytics_server/mhq/service/workflows/sync/etl_github_actions_handler.py (2)

203-204: LGTM: GitHub API service initialized with custom domain.

The GitHub API service is properly initialized with both the access token and custom domain.


1-206:

❓ Verification inconclusive

Fix Black formatting issues.

The file is flagged by the Black integration pipeline - please run Black to fix the formatting.


🏁 Script executed:

#!/bin/bash
# Run black to check formatting issues
black --check backend/analytics_server/mhq/service/workflows/sync/etl_github_actions_handler.py

Length of output: 193


Apply Black formatting
The CI indicates formatting issues in this file. Please run the following locally and commit the updated file:

black backend/analytics_server/mhq/service/workflows/sync/etl_github_actions_handler.py

Then push the reformatted file so the Black check passes.

🧰 Tools
🪛 Ruff (0.8.2)

70-73: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

🪛 GitHub Actions: Black Integration

[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.

backend/analytics_server/mhq/exapi/github.py (6)

34-38: LGTM: API URL helper method implementation.

This helper method correctly handles the differences between GitHub.com and GitHub Enterprise Server API URLs.


52-57: LGTM: check_pat method is using the base_url properly.

The check_pat method now correctly uses the base_url when making the API request, which will work with GitHub Enterprise.

🧰 Tools
🪛 Ruff (0.8.2)

56-56: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


157-161: LGTM: get_contributors method is using the base_url properly.

The method correctly uses the base_url to construct the GitHub API URL.


209-213: LGTM: get_org_members method is using the base_url properly.

The method correctly uses the base_url to construct the GitHub API URL.


247-255: LGTM: get_workflow_runs method is using the base_url properly.

The method correctly uses the base_url to construct the GitHub API URL.


1-277:

❓ Verification inconclusive

Fix Black formatting issues.

The file is flagged by the Black integration pipeline - please run Black to fix the formatting.


🏁 Script executed:

#!/bin/bash
# Run black to check formatting issues
black --check backend/analytics_server/mhq/exapi/github.py

Length of output: 156


Install and run Black to fix formatting issues

Our sandbox couldn’t verify formatting because black isn’t available here. Please ensure Black is installed in your environment and rerun:

pip install black
black --check backend/analytics_server/mhq/exapi/github.py
black backend/analytics_server/mhq/exapi/github.py

Once formatting passes, commit the changes.

🧰 Tools
🪛 Ruff (0.8.2)

56-56: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

🪛 GitHub Actions: Black Integration

[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.

web-server/pages/api/internal/[org_id]/utils.ts (1)

55-56: 🛠️ Refactor suggestion

Update the function call to use the new custom domain helper functions

This function still uses the hardcoded API URL from line 9, but should leverage the new getGitHubRestApiUrl helper function.

Update the function to use the helper:

const searchRepoWithURL = async (searchString: string) => {
  const apiUrl = await getGitHubRestApiUrl(`repos/${searchString}`);
  const response = await axios.get<GithubRepo>(apiUrl);
  const repo = response.data;
  return [
    // ... rest of code

Likely an incorrect or invalid review comment.

Comment on lines 321 to 322
return `${baseUrl}/${path}`.replace(/\/+/g, '/');
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Domain URL needs protocol verification

The code concatenates the custom domain with path segments, but doesn't ensure the custom domain includes a protocol.

Update the URL construction to ensure proper format:

const getGitHubRestApiUrl = async (path: string): Promise<string> => {
  const customDomain = await getGitHubCustomDomain();
-  const baseUrl = customDomain ? `https://${customDomain}/api/v3` : GITHUB_API_URL;
+  const baseUrl = customDomain 
+    ? (customDomain.startsWith('http') ? customDomain : `https://${customDomain}`) + '/api/v3' 
+    : GITHUB_API_URL;
  return `${baseUrl}/${path}`.replace(/\/+/g, '/');
};

Comment on lines 59 to +60
try {
const response = await axios.get('https://api.github.com', {
const response = await axios.get(baseUrl, {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix similar issues with URL construction in getMissingPATScopes

The same URL validation and normalization issues exist in this function.

Apply the same improvements:

const baseUrl = customDomain
-  ? `${customDomain}/api/v3`
+  ? `${normalizeCustomDomain(customDomain)}/api/v3`
  : DEFAULT_GH_URL;
try {
-  const response = await axios.get(baseUrl, {
+  const response = await axios.get(baseUrl.replace(/\/+/g, '/'), {
📝 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.

Suggested change
try {
const response = await axios.get('https://api.github.com', {
const response = await axios.get(baseUrl, {
// normalize customDomain and ensure no duplicate slashes when calling the API
const baseUrl = customDomain
? `${normalizeCustomDomain(customDomain)}/api/v3`
: DEFAULT_GH_URL;
try {
const response = await axios.get(
baseUrl.replace(/\/+/g, '/'),
{
// …original request options…
}
);
// …
} catch (err) {
// …
}

Comment on lines 39 to 41
const baseUrl = customDomain
? `${customDomain}/api/v3`
: DEFAULT_GH_URL;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add URL validation and normalization for custom domain

The code assumes the custom domain is properly formatted, but there's no validation or normalization.

Add validation and normalizing for the custom domain:

const baseUrl = customDomain
-  ? `${customDomain}/api/v3`
+  ? `${normalizeCustomDomain(customDomain)}/api/v3`
  : DEFAULT_GH_URL;

// Add this helper function at the bottom of the file
+ function normalizeCustomDomain(domain: string): string {
+   // Remove trailing slashes
+   domain = domain.replace(/\/+$/, '');
+   // Add protocol if missing
+   if (!domain.startsWith('http')) {
+     domain = `https://${domain}`;
+   }
+   return domain;
+ }
📝 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.

Suggested change
const baseUrl = customDomain
? `${customDomain}/api/v3`
: DEFAULT_GH_URL;
const baseUrl = customDomain
? `${normalizeCustomDomain(customDomain)}/api/v3`
: DEFAULT_GH_URL;
// …other code…
// Add this helper at the bottom of the file
function normalizeCustomDomain(domain: string): string {
// Remove trailing slashes
domain = domain.replace(/\/+$/, '');
// Add protocol if missing
if (!domain.startsWith('http')) {
domain = `https://${domain}`;
}
return domain;
}

Comment on lines +47 to +50
const handleDomainChange = (e: string) => {
customDomain.set(e);
showDomainError.set('');
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation function for custom domain

The handleDomainChange function should validate the domain format.

Add validation to ensure the domain is properly formatted:

const handleDomainChange = (e: string) => {
  customDomain.set(e);
  showDomainError.set('');
+  // Basic validation when a value is entered
+  if (e && !isValidDomain(e)) {
+    showDomainError.set('Please enter a valid domain');
+  }
};

+ // Helper function to validate domain format
+ const isValidDomain = (domain: string): boolean => {
+   // Allow empty value (optional field)
+   if (!domain) return true;
+   
+   // Basic domain validation - allow with or without protocol
+   const domainRegex = /^(https?:\/\/)?([a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?\.)+[a-zA-Z]{2,}(\/.*)?$/;
+   return domainRegex.test(domain);
+ };
📝 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.

Suggested change
const handleDomainChange = (e: string) => {
customDomain.set(e);
showDomainError.set('');
};
const handleDomainChange = (e: string) => {
customDomain.set(e);
showDomainError.set('');
// Basic validation when a value is entered
if (e && !isValidDomain(e)) {
showDomainError.set('Please enter a valid domain');
}
};
// Helper function to validate domain format
const isValidDomain = (domain: string): boolean => {
// Allow empty value (optional field)
if (!domain) return true;
// Basic domain validation - allow with or without protocol
const domainRegex = /^(https?:\/\/)?([a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?\.)+[a-zA-Z]{2,}(\/.*)?$/;
return domainRegex.test(domain);
};

Copy link
Contributor

@adnanhashmi09 adnanhashmi09 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any validation of custom domain input in the frontend. What if the user enters the url with https, what if there are paths. You should validate and normalise the input you receive from the end user.
How is this currently being done in gitlab? Do we not validate or extract the domain from the url?


if not github_domain:
LOG.warn(
f"Custom domain not found for intergration for org {org_id} and provider {UserIdentityProvider.GITLAB.value}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo and wrong enum

Suggested change
f"Custom domain not found for intergration for org {org_id} and provider {UserIdentityProvider.GITLAB.value}"
f"Custom domain not found for integration for org {org_id} and provider {UserIdentityProvider.GITHUB.value}"

@@ -22,11 +22,20 @@ class GithubRateLimitExceeded(Exception):


class GithubApiService:
def __init__(self, access_token: str):
def __init__(self, access_token: str, domain = "https://api.github.com"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type annotate domain variable
This would be Optional[str], i think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also instead of setting a default str in the argument. It would be cleaner to put it inside _get_api_url like in the following comment

Comment on lines 34 to 38
def _get_api_url(self, domain: str) -> str:
if domain == "https://api.github.com":
return domain
else:
return f"{domain}/api/v3"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def _get_api_url(self, domain: str) -> str:
if domain == "https://api.github.com":
return domain
else:
return f"{domain}/api/v3"
def _get_api_url(self, domain: Optional[str]) -> str:
if not domain:
return "https://api.github.com"
else:
return f"{domain}/api/v3"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way the linter is also satisfied

Comment on lines 172 to 191
def _get_custom_github_domain() -> Optional[str]:
DEFAULT_DOMAIN = "https://api.github.com"
core_repo_service = CoreRepoService()
integrations = core_repo_service.get_org_integrations_for_names(
org_id, [UserIdentityProvider.GITHUB.value]
)

github_domain = (
integrations[0].provider_meta.get("custom_domain")
if integrations[0].provider_meta
else None
)

if not github_domain:
LOG.warn(
f"Custom domain not found for intergration for org {org_id} and provider {UserIdentityProvider.GITLAB.value}"
)
return DEFAULT_DOMAIN

return github_domain
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets define this function in some util file for github. You can create a utils.py file in the code directory and define it there once without having to redefine the same function again. and then import it wherever you want to use it

Comment on lines 310 to 316
const getGitHubCustomDomain = async (): Promise<string | null> => {
const provider_meta = await db('Integration')
.where('name', Integration.GITHUB)
.then((r: Row<'Integration'>[]) => r.map((item) => item.provider_meta));

return head(provider_meta || [])?.custom_domain || null;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets have some error handling here. If the db query fails somehow it will fail silently. At least a log should be there, so that it can get got in the system logs


const getGitHubRestApiUrl = async (path: string): Promise<string> => {
const customDomain = await getGitHubCustomDomain();
const baseUrl = customDomain ? `https://${customDomain}/api/v3` : GITHUB_API_URL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong. How is the default rest API url = 'https://api.github.com/graphql'


const getGitHubGraphQLUrl = async (): Promise<string> => {
const customDomain = await getGitHubCustomDomain();
return customDomain ? `https://${customDomain}/api/graphql` : GITHUB_API_URL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly here. GITHUB_API_URL is an ambiguous name when we have both rest api and graphql api

Comment on lines 169 to 196
<FlexBox gap2 col>
<FlexBox alignBase gap1>
Custom domain
</FlexBox>
<TextField
id="github-custom-domain"
onKeyDown={(e) => {
if (e.key === 'Enter') {
e.preventDefault();
e.stopPropagation();
e.nativeEvent.stopImmediatePropagation();
handleSubmission();
return;
}
}}
error={!!showDomainError.value}
sx={{ width: '100%' }}
value={customDomain.value}
onChange={(e) => handleDomainChange(e.currentTarget.value)}
label={
isDomainInputFocus.value || customDomain.value
? 'Custom Domain'
: '(Optional)'
}
onFocus={isDomainInputFocus.true}
onBlur={isDomainInputFocus.false}
/>
</FlexBox>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can have a placeholder value to show what a custom domain input would look like. See this review

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🔭 Outside diff range comments (1)
backend/analytics_server/mhq/utils/github.py (1)

56-76: ⚠️ Potential issue

Fix potential IndexError and typo in warning message.

The function has two issues that need attention:

  1. It assumes integrations[0] exists, which would cause an IndexError if no GitHub integrations are found for the organization.
  2. There's a typo in the warning message: "intergration" should be "integration".
def get_custom_github_domain(org_id: str) -> Optional[str]:
    DEFAULT_DOMAIN = "https://api.github.com"
    core_repo_service = CoreRepoService()
    integrations = core_repo_service.get_org_integrations_for_names(
        org_id, [UserIdentityProvider.GITHUB.value]
    )

-    github_domain = (
-        integrations[0].provider_meta.get("custom_domain")
-        if integrations[0].provider_meta
-        else None
-    )
+    github_domain = None
+    if integrations and len(integrations) > 0:
+        github_domain = (
+            integrations[0].provider_meta.get("custom_domain")
+            if integrations[0].provider_meta
+            else None
+        )

    if not github_domain:
        LOG.warn(
-            f"Custom domain not found for intergration for org {org_id} and provider {UserIdentityProvider.GITHUB.value}"
+            f"Custom domain not found for integration for org {org_id} and provider {UserIdentityProvider.GITHUB.value}"
        )
        return DEFAULT_DOMAIN

    return github_domain
🧹 Nitpick comments (1)
backend/analytics_server/tests/exapi/test_github.py (1)

33-41: Add tests for additional domain edge cases.

While the current test for custom domains is good, consider adding tests for these edge cases:

  1. Domains without protocol (e.g., "github.example.com" instead of "https://github.example.com")
  2. Domains with trailing slashes (e.g., "https://github.example.com/")
  3. Invalid domains or malformed URLs

These tests would strengthen the robustness of the implementation.

Example test for a domain without protocol:

@patch("mhq.exapi.github.Github", new=DummyGithub)
def test_domain_without_protocol_prepends_https(self):
    token = "deadpool"
    custom_domain = "github.sujai.com"  # No protocol
    service = GithubApiService(access_token=token, domain=custom_domain)
    # Verify if the implementation handles this correctly
    # Either by prepending https:// or by validating input
    expected = f"https://{custom_domain}/api/v3"
    self.assertEqual(service.base_url, expected)
    self.assertEqual(service._g.base_url, expected)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2dbf898 and bafd72f.

📒 Files selected for processing (8)
  • backend/analytics_server/mhq/exapi/github.py (1 hunks)
  • backend/analytics_server/mhq/service/code/sync/etl_github_handler.py (2 hunks)
  • backend/analytics_server/mhq/service/workflows/sync/etl_github_actions_handler.py (2 hunks)
  • backend/analytics_server/mhq/utils/github.py (2 hunks)
  • backend/analytics_server/tests/exapi/test_github.py (1 hunks)
  • web-server/pages/api/internal/[org_id]/utils.ts (4 hunks)
  • web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx (6 hunks)
  • web-server/src/utils/auth.ts (3 hunks)
✅ Files skipped from review due to trivial changes (1)
  • backend/analytics_server/mhq/service/workflows/sync/etl_github_actions_handler.py
🚧 Files skipped from review as they are similar to previous changes (5)
  • backend/analytics_server/mhq/service/code/sync/etl_github_handler.py
  • web-server/src/utils/auth.ts
  • backend/analytics_server/mhq/exapi/github.py
  • web-server/pages/api/internal/[org_id]/utils.ts
  • web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/analytics_server/tests/exapi/test_github.py (1)
backend/analytics_server/mhq/exapi/github.py (1)
  • GithubApiService (24-273)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: All file linting
🔇 Additional comments (3)
backend/analytics_server/mhq/utils/github.py (1)

3-3: Appropriate import additions.

The new imports properly support the added functionality for handling custom GitHub domains. The Optional type hint is correctly used in the function signature.

Also applies to: 8-9

backend/analytics_server/tests/exapi/test_github.py (2)

1-12: Good test setup with appropriate mocking.

The test file has a well-structured setup with the DummyGithub mock class and proper imports. The mock class effectively captures the constructor parameters for verification.


14-32: Tests cover basic domain handling scenarios.

The tests verify essential behaviors for both default and empty domain cases, ensuring the service correctly falls back to the standard GitHub API URL.

@thesujai
Copy link
Contributor Author

Hi @adnanhashmi09,
Can you give another look?


const getGitHubGraphQLUrl = async (): Promise<string> => {
const customDomain = await getGitHubCustomDomain();
return customDomain ? `https://${customDomain}/api/graphql` : GITHUB_API_URL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return customDomain ? `https://${customDomain}/api/graphql` : GITHUB_API_URL;
return customDomain ? `https://${customDomain}/api/graphql` : `${GITHUB_API_URL}/graphql`;

Comment on lines 57 to 61
const checkDomainWithRegex = (domain: string) => {
const regex =
/^(https?:\/\/)[a-zA-Z0-9]+([-.][a-zA-Z0-9]+)*\.[a-zA-Z]{2,}(:[0-9]{1,5})?(\/.*)?$/;
return regex.test(domain);
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must include test for this. Very difficult to understand what this regex does. Not sure what the logic is.

@@ -3,6 +3,8 @@ import { isNil, reject } from 'ramda';

import { Integration } from '@/constants/integrations';

const DEFAULT_GH_URL = 'https://api.github.com';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can have this defined in a constant.ts file as this is being used in multiple places

@thesujai thesujai force-pushed the custom-gh-domain branch from 36a862b to 8d665f0 Compare May 2, 2025 06:19
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 2

🔭 Outside diff range comments (1)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (1)

1-85: 🛠️ Refactor suggestion

Add test for organization-specific domains

Based on the PR objectives to support GitHub Enterprise Server, it would be valuable to test that the utility functions correctly handle organization-specific domains.

Add a test case that verifies organization-specific domain resolution:

it('retrieves custom domain for specific organization', async () => {
  const orgId = 'test-org-123';
  const mockMeta = [{ custom_domain: 'org.github.enterprise.com' }];
  
  (db as jest.Mock).mockReturnValue({
    where: jest.fn().mockImplementation((field, value) => {
      expect(field).toBe('org_id');
      expect(value).toBe(orgId);
      return {
        where: jest.fn().mockReturnThis(),
        then: jest.fn().mockResolvedValue(mockMeta)
      };
    })
  });

  const domain = await githubUtils.getGitHubCustomDomain(orgId);
  expect(domain).toBe('org.github.enterprise.com');
});
🧰 Tools
🪛 Biome (1.9.4)

[error] 19-19: Do not add then to an object.

(lint/suspicious/noThenProperty)


[error] 29-29: Do not add then to an object.

(lint/suspicious/noThenProperty)

♻️ Duplicate comments (3)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (3)

27-30: Avoid using 'then' method in mock objects

Same issue as before with the 'then' property in mock objects.

🧰 Tools
🪛 Biome (1.9.4)

[error] 29-29: Do not add then to an object.

(lint/suspicious/noThenProperty)


55-55: Test might fail due to typo in DEFAULT_GH_URL

This test would fail because DEFAULT_GH_URL contains a typo ('githb' instead of 'github'). The test is correctly written, but the constant it depends on has an error.


75-75: Test might fail due to typo in DEFAULT_GH_URL

Same issue as before with the typo in DEFAULT_GH_URL affecting test expectations.

🧹 Nitpick comments (3)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (3)

17-20: Avoid using 'then' method in mock objects

The static analysis tools flagged the use of the 'then' property on objects. Instead of mocking with a 'then' method that returns a Promise, use Jest's mockResolvedValue directly on the entire chain.

Here's a cleaner approach:

-(db as jest.Mock).mockReturnValue({
-  where: jest.fn().mockReturnThis(),
-  then: jest.fn().mockResolvedValue(mockMeta)
-});
+(db as jest.Mock).mockReturnValue({
+  where: jest.fn().mockReturnThis(),
+});
+(db().where().mockResolvedValue(mockMeta));

Or alternatively:

-(db as jest.Mock).mockReturnValue({
-  where: jest.fn().mockReturnThis(),
-  then: jest.fn().mockResolvedValue(mockMeta)
-});
+(db as jest.Mock).mockImplementation(() => ({
+  where: jest.fn().mockImplementation(() => Promise.resolve(mockMeta))
+}));
🧰 Tools
🪛 Biome (1.9.4)

[error] 19-19: Do not add then to an object.

(lint/suspicious/noThenProperty)


45-46: Typo in error log message

There's a typo in the error message: "Error occured" should be "Error occurred".

-'Error occured while getting custom domain from database:',
+'Error occurred while getting custom domain from database:',

1-85: Add test for URL protocol handling

The tests cover domain normalization for trailing slashes, but don't verify how the utils handle URLs with or without protocol prefixes.

Consider adding a test case that verifies protocol handling:

it('handles domains with and without protocol prefixes', async () => {
  // Test with protocol included
  jest.spyOn(githubUtils, 'getGitHubCustomDomain').mockResolvedValue('https://git.example.com');
  let url = await githubUtils.getGitHubRestApiUrl('repos/owner/repo');
  expect(url).toBe('https://git.example.com/api/v3/repos/owner/repo');
  
  // Test without protocol
  jest.spyOn(githubUtils, 'getGitHubCustomDomain').mockResolvedValue('git.example.com');
  url = await githubUtils.getGitHubRestApiUrl('repos/owner/repo');
  expect(url).toBe('https://git.example.com/api/v3/repos/owner/repo');
});
🧰 Tools
🪛 Biome (1.9.4)

[error] 19-19: Do not add then to an object.

(lint/suspicious/noThenProperty)


[error] 29-29: Do not add then to an object.

(lint/suspicious/noThenProperty)

🛑 Comments failed to post (2)
web-server/src/utils/domainCheck.ts (1)

1-5: 🛠️ Refactor suggestion

Add JSDoc comments and consider expanding domain validation patterns.

The checkDomainWithRegex function provides a good foundation for domain validation, but has some limitations:

  1. The current regex doesn't support:

  2. Missing documentation that explains the function's purpose and expectations.

Consider adding JSDoc comments and expanding the regex pattern:

+/**
+ * Validates if a string is a properly formatted domain URL
+ * @param domain - The domain URL to validate (must include http:// or https://)
+ * @returns boolean indicating if the domain is valid
+ */
 export const checkDomainWithRegex = (domain: string) => {
   const regex =
-    /^(https?:\/\/)[A-Za-z0-9]+([-.][A-Za-z0-9]+)*\.[A-Za-z]{2,}(:[0-9]{1,5})?(\/\S*)?$/;
+    /^(https?:\/\/)((localhost)|([A-Za-z0-9]+([-.][A-Za-z0-9]+)*\.[A-Za-z0-9]{2,})|((\d{1,3}\.){3}\d{1,3}))(:[0-9]{1,5})?(\/\S*)?$/;
   return regex.test(domain);
 };

This enhanced regex adds support for localhost, IP addresses, and more flexible TLDs while maintaining the requirement for HTTP/HTTPS schemes.

web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (1)

7-7: ⚠️ Potential issue

Typo in the DEFAULT_GH_URL constant

The imported DEFAULT_GH_URL constant has a typo in its definition. It's defined as 'https://api.githb.com' (missing an 'u') according to the relevant code snippet, which would cause API requests to fail.

Make sure to fix the URL in the constants file:

// In web-server/src/constants/urls.ts
-export const DEFAULT_GH_URL = 'https://api.githb.com';
+export const DEFAULT_GH_URL = 'https://api.github.com';

Committable suggestion skipped: line range outside the PR's diff.

@thesujai
Copy link
Contributor Author

thesujai commented May 2, 2025

requested changes addressed 👍

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (1)

27-30: Same mock implementation issue as above

This has the same issue with mocking the then property directly.

🧰 Tools
🪛 Biome (1.9.4)

[error] 29-29: Do not add then to an object.

(lint/suspicious/noThenProperty)

🧹 Nitpick comments (3)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (3)

17-20: Mock implementation needs attention

The static analysis tool flagged the use of the then property in your mock. While this works for the test, it's generally not recommended to mock then directly as it can lead to unexpected behavior and makes the test less maintainable.

Consider refactoring to use mockResolvedValue directly on the query chain:

-(db as jest.Mock).mockReturnValue({
-  where: jest.fn().mockReturnThis(),
-  then: jest.fn().mockResolvedValue(mockMeta)
-});
+(db as jest.Mock).mockReturnValue({
+  where: jest.fn().mockReturnThis(),
+  mockResolvedValue: jest.fn().mockResolvedValue(mockMeta)
+});

Alternatively, structure your mock to better reflect Knex's query builder pattern:

+(db as jest.Mock).mockReturnValue({
+  where: jest.fn().mockReturnThis(),
+  then: jest.fn((callback) => Promise.resolve(callback(mockMeta)))
+});
🧰 Tools
🪛 Biome (1.9.4)

[error] 19-19: Do not add then to an object.

(lint/suspicious/noThenProperty)


45-46: Fix typo in error message

There's a typo in the error message: "occured" should be "occurred".

-'Error occured while getting custom domain from database:',
+'Error occurred while getting custom domain from database:',

9-12: Consider adding beforeEach for common test setup

You're resetting mocks after each test, but you may want to consider setting up common mocks in a beforeEach block to reduce repetition across test cases.

describe('GitHub URL utilities', () => {
  afterEach(() => {
    jest.resetAllMocks();
  });
+
+  beforeEach(() => {
+    // Common setup for all tests can go here
+  });
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 36a862b and 8d665f0.

📒 Files selected for processing (10)
  • web-server/jest.config.js (1 hunks)
  • web-server/jest.setup.js (1 hunks)
  • web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (1 hunks)
  • web-server/pages/api/internal/[org_id]/utils.ts (4 hunks)
  • web-server/src/constants/urls.ts (1 hunks)
  • web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx (7 hunks)
  • web-server/src/content/Dashboards/ConfigureGitlabModalBody.tsx (1 hunks)
  • web-server/src/utils/__tests__/domainCheck.test.ts (1 hunks)
  • web-server/src/utils/auth.ts (3 hunks)
  • web-server/src/utils/domainCheck.ts (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • web-server/src/utils/tests/domainCheck.test.ts
  • web-server/src/constants/urls.ts
🚧 Files skipped from review as they are similar to previous changes (7)
  • web-server/src/content/Dashboards/ConfigureGitlabModalBody.tsx
  • web-server/jest.setup.js
  • web-server/jest.config.js
  • web-server/src/utils/domainCheck.ts
  • web-server/pages/api/internal/[org_id]/utils.ts
  • web-server/src/utils/auth.ts
  • web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (1)
web-server/src/constants/urls.ts (1)
  • DEFAULT_GH_URL (1-1)
🪛 Biome (1.9.4)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts

[error] 19-19: Do not add then to an object.

(lint/suspicious/noThenProperty)


[error] 29-29: Do not add then to an object.

(lint/suspicious/noThenProperty)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: All file linting
🔇 Additional comments (3)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (3)

1-84: Test coverage looks good for GitHub Enterprise integration

The tests cover the key scenarios for working with custom GitHub domains, including error handling and URL normalization. This provides good coverage for the GitHub Enterprise Server integration feature.

🧰 Tools
🪛 Biome (1.9.4)

[error] 19-19: Do not add then to an object.

(lint/suspicious/noThenProperty)


[error] 29-29: Do not add then to an object.

(lint/suspicious/noThenProperty)


78-82:

✅ Verification successful

Verify GraphQL API URL format for GitHub Enterprise

Similarly, the test expects the custom domain GraphQL URL to be https://domain/api/graphql. Confirm this matches the actual GitHub Enterprise GraphQL endpoint structure.


🌐 Web query:

What is the correct GraphQL API URL format for GitHub Enterprise Server?

💡 Result:

The correct GraphQL API URL format for GitHub Enterprise Server is:

https://<HOSTNAME>/api/graphql

Replace <HOSTNAME> with the network address or domain of your GitHub Enterprise Server instance. For example, if your GitHub Enterprise Server is available at github.company.com, your GraphQL endpoint would be:

https://github.company.com/api/graphql

This endpoint is used for all GraphQL operations—both queries and mutations—and does not change based on the type of data you are requesting[3][5][6].

Citations:


Verified GraphQL API URL format is correct

The test’s expectation of https://api.github.local/api/graphql matches the official GitHub Enterprise Server GraphQL endpoint structure (https://<HOSTNAME>/api/graphql). No changes required.


58-62:

✅ Verification successful

Verify REST API URL format for GitHub Enterprise

The test expects the custom domain REST API URL to follow the format https://domain/api/v3/path. Make sure this matches GitHub Enterprise's actual API URL structure, as some versions or configurations might use different paths.


🌐 Web query:

What is the correct API URL format for GitHub Enterprise Server?

💡 Result:

The correct API URL format for GitHub Enterprise Server (GHES) is:

http(s)://HOSTNAME/api/v3/PATH
  • Replace HOSTNAME with your GitHub Enterprise Server instance's hostname.
  • Replace PATH with the specific API endpoint you wish to access, such as users, repos, or another resource path[4][9].

Examples:

  • List all users:
    https://ghe.example.com/api/v3/users
  • Get repository details:
    https://ghe.example.com/api/v3/repos/OWNER/REPO

Notes:

  • For most GHES REST API endpoints, this base URL format is used[4][9].
  • For some server administration tasks (using the Manage GitHub Enterprise Server API), include the relevant port (typically 8443 for TLS or 8080 for non-TLS), for example:
    https://ghe.example.com:8443/manage[2][4].
  • For regular repository and user endpoints, use the base /api/v3/ prefix as shown above[4][9].

This URL format ensures compatibility with the GHES REST API and should be used in API requests to your enterprise instance.

Citations:


Confirmed correct GitHub Enterprise REST API URL format

The test’s expectation https://git.sujai.com/api/v3/repos/owner/repo matches the documented GitHub Enterprise Server API structure (https://HOSTNAME/api/v3/PATH). No changes needed.


import { db } from '@/utils/db';
import * as githubUtils from '../utils';
import { DEFAULT_GH_URL } from '@/constants/urls';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical issue with DEFAULT_GH_URL constant

There appears to be a typo in the imported constant from @/constants/urls.ts. The constant is defined as 'https://api.githb.com' (missing an 'u'), which will cause all tests and production code to use an incorrect GitHub API URL.

#!/bin/bash
# Verify the constant definition in the urls.ts file
cat web-server/src/constants/urls.ts | grep DEFAULT_GH_URL

@thesujai thesujai force-pushed the custom-gh-domain branch from 8d665f0 to cad13c0 Compare May 2, 2025 06:23
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (1)

45-46: Minor typo in error message.

There's a small typo in the error message: "Error occured while getting" should be "Error occurred while getting".

-        'Error occured while getting custom domain from database:',
+        'Error occurred while getting custom domain from database:',
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8d665f0 and cad13c0.

📒 Files selected for processing (10)
  • web-server/jest.config.js (1 hunks)
  • web-server/jest.setup.js (1 hunks)
  • web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (1 hunks)
  • web-server/pages/api/internal/[org_id]/utils.ts (4 hunks)
  • web-server/src/constants/urls.ts (1 hunks)
  • web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx (7 hunks)
  • web-server/src/content/Dashboards/ConfigureGitlabModalBody.tsx (1 hunks)
  • web-server/src/utils/__tests__/domainCheck.test.ts (1 hunks)
  • web-server/src/utils/auth.ts (3 hunks)
  • web-server/src/utils/domainCheck.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • web-server/src/utils/tests/domainCheck.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
  • web-server/jest.setup.js
  • web-server/jest.config.js
  • web-server/src/content/Dashboards/ConfigureGitlabModalBody.tsx
  • web-server/src/constants/urls.ts
  • web-server/src/utils/domainCheck.ts
  • web-server/src/utils/auth.ts
  • web-server/pages/api/internal/[org_id]/utils.ts
  • web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (1)
web-server/src/constants/urls.ts (1)
  • DEFAULT_GH_URL (1-1)
🪛 Biome (1.9.4)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts

[error] 19-19: Do not add then to an object.

(lint/suspicious/noThenProperty)


[error] 29-29: Do not add then to an object.

(lint/suspicious/noThenProperty)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: All file linting
🔇 Additional comments (4)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (4)

7-7: Previous issue with DEFAULT_GH_URL has been fixed.

The constant is now correctly imported from @/constants/urls.ts and based on the provided code snippet, it contains the proper URL 'https://api.github.com'.


14-49: Well-structured test coverage for the custom domain retrieval.

The tests thoroughly cover the key scenarios for getGitHubCustomDomain:

  • Successfully retrieving a custom domain
  • Handling empty results
  • Error handling with appropriate logging

This is essential for GitHub Enterprise support and ensures reliable fallback behavior.

Note: The static analysis warnings about "Do not add then to an object" (lines 19, 29) are false positives—this is standard Jest mocking syntax for chained methods.

🧰 Tools
🪛 Biome (1.9.4)

[error] 19-19: Do not add then to an object.

(lint/suspicious/noThenProperty)


[error] 29-29: Do not add then to an object.

(lint/suspicious/noThenProperty)


51-69: Good test coverage for REST API URL construction.

The tests effectively verify:

  • Default URL fallback when no custom domain exists
  • Custom domain integration with the correct API path (/api/v3/)
  • URL normalization to handle potential path inconsistencies

This ensures robust URL handling for GitHub Enterprise environments.


71-83: Proper GraphQL endpoint handling for GitHub Enterprise.

The tests correctly verify that:

  • The default GraphQL endpoint is used when no custom domain is provided
  • Custom domains use the appropriate GraphQL path (/api/graphql) for GitHub Enterprise

This completes the URL utilities testing needed for GitHub Enterprise support.

@adnanhashmi09 adnanhashmi09 merged commit 8d4a3db into middlewarehq:main May 9, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow changing GitHub server hostname (for self-hosted/enterprise instances)
2 participants