Support for github enterprise server(self hosted github) #646
Conversation
WalkthroughThe 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 Changes
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
Assessment against linked issues
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
thesujai
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.pyLength 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.pyLength 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.pyLength of output: 225
Please fix Black formatting issues
The file
backend/analytics_server/mhq/service/code/sync/etl_github_handler.pyis 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.pyAfter 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 suggestionUpdate dependency array for handleSubmission
The dependency array for
handleSubmissionis missingcustomDomain.valueRefwhich 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
domainand removal of trailing whitespace.web-server/pages/api/internal/[org_id]/utils.ts (1)
324-327: Apply URL sanitization to GraphQL endpointUnlike 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 constructionWhen 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 arrayThe
getMissingPATScopesfunction 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 parameterThe code sends
custom_domainas 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
📒 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.pyLength 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.pyThen push the reformatted file so the Black check passes.
🧰 Tools
🪛 Ruff (0.8.2)
70-73: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto 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_patmethod 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
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto 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.pyLength of output: 156
Install and run Black to fix formatting issues
Our sandbox couldn’t verify formatting because
blackisn’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.pyOnce formatting passes, commit the changes.
🧰 Tools
🪛 Ruff (0.8.2)
56-56: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto 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 suggestionUpdate 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
getGitHubRestApiUrlhelper 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 codeLikely an incorrect or invalid review comment.
| 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 |
There was a problem hiding this comment.
Fix incorrect provider name in log message.
The warning message incorrectly refers to GitLab instead of GitHub.
- f"Custom domain not found for intergration for org {org_id} and provider {UserIdentityProvider.GITLAB.value}"
+ f"Custom domain not found for intergration for org {org_id} and provider {UserIdentityProvider.GITHUB.value}"Also, there's a typo in "intergration" that should be "integration".
Committable suggestion skipped: line range outside the PR's diff.
| 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 |
There was a problem hiding this comment.
Fix incorrect provider name in log message.
The warning message incorrectly refers to GitLab instead of GitHub.
- f"Custom domain not found for intergration for org {org_id} and provider {UserIdentityProvider.GITLAB.value}"
+ f"Custom domain not found for intergration for org {org_id} and provider {UserIdentityProvider.GITHUB.value}"Also, there's a typo in "intergration" that should be "integration".
Committable suggestion skipped: line range outside the PR's diff.
| 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; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for custom domain retrieval
The getGitHubCustomDomain function lacks error handling for database queries or malformed domain values, unlike the similar replaceURL function for GitLab (lines 291-308).
Add a try-catch block to handle potential errors:
const getGitHubCustomDomain = async (): Promise<string | null> => {
+ try {
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;
+ } catch (error) {
+ console.error('Error retrieving GitHub custom domain:', error);
+ return null;
+ }
};📝 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.
| 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; | |
| }; | |
| const getGitHubCustomDomain = async (): Promise<string | null> => { | |
| try { | |
| 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; | |
| } catch (error) { | |
| console.error('Error retrieving GitHub custom domain:', error); | |
| return null; | |
| } | |
| }; |
| return `${baseUrl}/${path}`.replace(/\/+/g, '/'); | ||
| }; |
There was a problem hiding this comment.
🛠️ 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, '/');
};| const getGitHubGraphQLUrl = async (): Promise<string> => { | ||
| const customDomain = await getGitHubCustomDomain(); | ||
| return customDomain ? `https://${customDomain}/api/graphql` : GITHUB_API_URL; | ||
| }; |
There was a problem hiding this comment.
Correct the default GraphQL API URL
The function uses GITHUB_API_URL as the default GraphQL URL, but this constant (line 9) is set to "https://api.github.com/graphql". However, the function adds "/api/graphql" for custom domains, creating inconsistency.
Fix the inconsistency:
// Line 9
-const GITHUB_API_URL = 'https://api.github.com/graphql';
+const GITHUB_REST_API_URL = 'https://api.github.com';
+const GITHUB_GRAPHQL_API_URL = 'https://api.github.com/graphql';
// In getGitHubRestApiUrl function (line 321)
- const baseUrl = customDomain ? `https://${customDomain}/api/v3` : GITHUB_API_URL;
+ const baseUrl = customDomain ? `https://${customDomain}/api/v3` : GITHUB_REST_API_URL;
// In getGitHubGraphQLUrl function (line 326)
- return customDomain ? `https://${customDomain}/api/graphql` : GITHUB_API_URL;
+ return customDomain ? `https://${customDomain}/api/graphql` : GITHUB_GRAPHQL_API_URL;📝 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.
| const getGitHubGraphQLUrl = async (): Promise<string> => { | |
| const customDomain = await getGitHubCustomDomain(); | |
| return customDomain ? `https://${customDomain}/api/graphql` : GITHUB_API_URL; | |
| }; | |
| const getGitHubGraphQLUrl = async (): Promise<string> => { | |
| const customDomain = await getGitHubCustomDomain(); | |
| return customDomain | |
| ? `https://${customDomain}/api/graphql` | |
| : GITHUB_GRAPHQL_API_URL; | |
| }; |
| try { | ||
| const response = await axios.get('https://api.github.com', { | ||
| const response = await axios.get(baseUrl, { |
There was a problem hiding this comment.
🛠️ 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.
| 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) { | |
| // … | |
| } |
| const baseUrl = customDomain | ||
| ? `${customDomain}/api/v3` | ||
| : DEFAULT_GH_URL; |
There was a problem hiding this comment.
🛠️ 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.
| 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; | |
| } |
| <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> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and helper text for custom domain input
The custom domain input lacks validation and user guidance on expected format.
Add validation and helper text:
<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}
+ helperText={isDomainInputFocus.value || customDomain.value ? "Example: github.mycompany.com" : ""}
+ placeholder="github.mycompany.com"
/>📝 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.
| <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> | |
| <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} | |
| helperText={isDomainInputFocus.value || customDomain.value ? "Example: github.mycompany.com" : ""} | |
| placeholder="github.mycompany.com" | |
| /> | |
| </FlexBox> |
| const handleDomainChange = (e: string) => { | ||
| customDomain.set(e); | ||
| showDomainError.set(''); | ||
| }; |
There was a problem hiding this comment.
🛠️ 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.
| 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); | |
| }; |
| const isDomainInputFocus = useBoolState(false); | ||
|
|
||
| const focusDomainInput = useCallback(() => { | ||
| if (!customDomain.value) | ||
| document.getElementById('gitlab-custom-domain')?.focus(); | ||
| else handleSubmission(); | ||
| }, [customDomain.value, handleSubmission]); |
There was a problem hiding this comment.
Fix incorrect input ID reference and potential circular function calls
There are two issues in this code:
- Line 117 references an input with ID "gitlab-custom-domain" but the actual ID used is "github-custom-domain"
- A potential circular reference exists between
focusDomainInputandhandleSubmission
Fix these issues:
const isDomainInputFocus = useBoolState(false);
const focusDomainInput = useCallback(() => {
if (!customDomain.value)
- document.getElementById('gitlab-custom-domain')?.focus();
+ document.getElementById('github-custom-domain')?.focus();
- else handleSubmission();
+ else {
+ // Only call handleSubmission if it wasn't called directly
+ if (!isLoading.value) {
+ handleSubmission();
+ }
+ }
}, [customDomain.value, handleSubmission, isLoading.value]);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
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}" |
There was a problem hiding this comment.
typo and wrong enum
| 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"): | |||
There was a problem hiding this comment.
type annotate domain variable
This would be Optional[str], i think?
There was a problem hiding this comment.
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
| def _get_api_url(self, domain: str) -> str: | ||
| if domain == "https://api.github.com": | ||
| return domain | ||
| else: | ||
| return f"{domain}/api/v3" |
There was a problem hiding this comment.
| 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" | |
There was a problem hiding this comment.
This way the linter is also satisfied
| 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 |
There was a problem hiding this comment.
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
| 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; | ||
| }; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
similarly here. GITHUB_API_URL is an ambiguous name when we have both rest api and graphql api
| <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> |
There was a problem hiding this comment.
can have a placeholder value to show what a custom domain input would look like. See this review
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
Enhancements
Bug Fixes