fix: Refactor gh integration #288
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of GitHub-related components and workflows, transitioning from an organization-specific model to a more generalized GitHub entity model. The changes span multiple files across different applications, focusing on enhancing flexibility and modularity in handling GitHub integrations. Key modifications include renaming components, updating type definitions, and modifying database schemas to support both organization and user entities. Changes
Sequence DiagramsequenceDiagram
participant User
participant WebApp
participant GitHub
participant Database
User->>WebApp: Initiate GitHub Integration
WebApp->>GitHub: Fetch Installation Details
GitHub-->>WebApp: Return Installation Data
WebApp->>Database: Create GitHub Entity
Database-->>WebApp: Confirm Entity Creation
WebApp->>User: Display Connected Entities
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (1)
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/GithubRemoveEntityDialog.tsx (1)
Line range hint
42-47: Update dialog text to reflect entity-based model.The dialog text still refers specifically to "Github organization" which doesn't align with the new entity-based model that supports both organizations and users.
- Are you sure you want to disconnect this Github organization? + Are you sure you want to disconnect this Github {githubEntity.type}? - Disconnecting the organization will remove the connection between - Ctrlplane and your Github organization for this workspace. + Disconnecting this {githubEntity.type} will remove the connection between + Ctrlplane and your Github {githubEntity.type} for this workspace.
🧹 Nitpick comments (7)
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/SelectPreconnectedEntityDialogContent.tsx (2)
48-49: Specify mutation input fields explicitlyWhile using
...entityspreads all properties intomutateAsync, explicitly specifying the required fields enhances clarity and security by ensuring only intended data is sent.Apply this diff to specify the input fields explicitly:
await githubEntityCreate.mutateAsync({ - ...entity, + installationId: entity.installationId, + type: entity.type, + slug: entity.slug, + avatarUrl: entity.avatarUrl, workspaceId, });
103-104: Handle undefinedavatarUrlinAvatarImageIf
avatarUrlcan beundefined, provide a fallback image or handle thenullcase to prevent rendering issues in theAvatarImagecomponent.apps/webservice/src/components/form/job-agent/JobAgentGitHubConfig.tsx (2)
95-98: Specify the radix parameter inNumber.parseInt.When using
Number.parseInt, it's recommended to specify the radix to avoid unexpected results due to different numeral systems.Apply this diff to specify the radix:
onSelect={(currentValue) => { onChange({ ...value, - workflowId: Number.parseInt(currentValue), + workflowId: Number.parseInt(currentValue, 10), }); setWorkflowOpen(false); }}
112-116: Consider adding validation for therefinput.The
refinput allows users to specify a branch or tag. Adding validation can help prevent errors due to invalid or mistyped references.You might implement validation logic like:
<Input placeholder="Ref (uses default branch if not set)" className="w-80" value={value.ref ?? ""} - onChange={(e) => onChange({ ...value, ref: e.target.value })} + onChange={(e) => { + const ref = e.target.value.trim(); + // Simple validation example: ensure ref is not empty if provided + onChange({ ...value, ref: ref || undefined }); + }} />Alternatively, you could provide feedback to the user if the ref is invalid.
apps/event-worker/src/job-dispatch/github.ts (1)
154-154: Ensure sensitive data is not logged.When logging, verify that no sensitive information, such as tokens or confidential identifiers, is included in the log output.
Review the logging statement to ensure compliance:
logger.info(`Creating workflow dispatch for job ${je.id}...`, { owner: parsedConfig.owner, repo: parsedConfig.repo, workflow_id: parsedConfig.workflowId, ref, - inputs: { job_id: je.id }, + // Avoid logging sensitive input data if applicable });If
inputscontain only non-sensitive data, it's acceptable. Otherwise, consider omitting or masking sensitive details.apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/GithubAddEntityDialog.tsx (1)
Line range hint
35-128: Consider improving UX text for entity type context.While the implementation is correct, the dialog title and descriptions still use organization-specific language in some places. Consider updating these to reflect the entity type context.
- <DialogTitle>Connect a new Organization</DialogTitle> + <DialogTitle>Connect a new GitHub Entity</DialogTitle> - Install the ctrlplane Github app on an organization to connect + Install the ctrlplane Github app on a GitHub organization or user to connect - <strong>Connect a new organization:</strong> Install the + <strong>Connect a new entity:</strong> Install the - <strong>Select pre-connected:</strong> Select a - pre-connected organization where the ctrlplane app is + <strong>Select pre-connected:</strong> Select an already + pre-connected entity where the ctrlplane app isapps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/GithubConnectedEntities.tsx (1)
97-128: Consider adding entity type indicator in the UI.While the implementation correctly handles both types of entities, it might be helpful to visually distinguish between organizations and users in the UI.
<p className="font-semibold text-neutral-200"> - {entity.slug} + {entity.slug} + <span className="ml-2 text-xs text-neutral-400"> + ({entity.type}) + </span> </p>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
apps/event-worker/src/job-dispatch/github.ts(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/CreateDeployment.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/CreateHookDialog.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/EditHookDialog.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/runbooks/create/CreateRunbookForm.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/EntityActionDropdown.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/GithubAddEntityDialog.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/GithubConnectedEntities.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/GithubRemoveEntityDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/SelectPreconnectedEntityDialogContent.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/page.tsx(2 hunks)apps/webservice/src/app/api/github/installation/route.ts(2 hunks)apps/webservice/src/app/api/github/octokit.ts(1 hunks)apps/webservice/src/components/form/job-agent/JobAgentConfig.tsx(2 hunks)apps/webservice/src/components/form/job-agent/JobAgentGitHubConfig.tsx(2 hunks)packages/api/src/router/github.ts(8 hunks)packages/db/drizzle/0061_left_jocasta.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/github.ts(3 hunks)packages/db/src/schema/job.ts(1 hunks)packages/validators/src/github/index.ts(1 hunks)
💤 Files with no reviewable changes (4)
- apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/CreateHookDialog.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/runbooks/create/CreateRunbookForm.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/hooks/EditHookDialog.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/_components/CreateDeployment.tsx
🧰 Additional context used
📓 Path-based instructions (15)
packages/validators/src/github/index.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/page.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/EntityActionDropdown.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/GithubRemoveEntityDialog.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/api/github/octokit.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/event-worker/src/job-dispatch/github.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/db/src/schema/github.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/GithubAddEntityDialog.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/SelectPreconnectedEntityDialogContent.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/components/form/job-agent/JobAgentConfig.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/components/form/job-agent/JobAgentGitHubConfig.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/db/src/schema/job.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/api/github/installation/route.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/api/src/router/github.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/GithubConnectedEntities.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (34)
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/SelectPreconnectedEntityDialogContent.tsx (6)
19-25: Type definition accurately reflects the GitHub entity structureThe
PreconnectedEntityComboboxPropstype appropriately definesgithubEntitieswith the necessary properties for both users and organizations.
32-34: Destructure all necessary props fromPreconnectedEntityComboboxPropsThe
githubUserprop is defined inPreconnectedEntityComboboxPropsbut is not being destructured in the component's parameters. IfgithubUseris needed within the component, ensure it is included in the destructured props.
41-41: Mutation for GitHub entity creation is correctly initializedThe
githubEntityCreatemutation is properly set up using the TRPC API.
45-46: Proper handling of missing selected entityThe code correctly checks if the selected entity exists before proceeding, preventing potential errors.
96-96: VerifyavatarUrlbefore updating stateEnsure that
avatarUrlis defined before callingsetImage(avatarUrl)to prevent setting the image state toundefined.
124-124: Button disabled state is correctly managedThe
disabledprop appropriately disables the button when no entity is selected or when the mutation is pending.apps/webservice/src/app/api/github/installation/route.ts (1)
165-171: Verify thatjobAgentconfiguration aligns with the new entity structure.Ensure that the
jobAgentis created with the correct configuration fields matching the updatedgithubEntitystructure.Run the following script to check the
jobAgentconfiguration:Replace
your_databasewith the actual database name. This script checks that theinstallationIdandownerinjobAgent.configmatch entries ingithub_entity.✅ Verification successful
Configuration structure is correctly aligned ✅
The jobAgent configuration correctly maps the entity fields:
config.installationIdmaps toentity.installationIdconfig.ownermaps toentity.slugThis structure is consistently implemented across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that job agents are configured correctly with the updated githubEntity. # Test: Retrieve all job agents and check their configurations. psql -d your_database -c " SELECT job_agent.name, job_agent.config->>'installationId' AS installation_id, job_agent.config->>'owner' AS owner FROM job_agent JOIN github_entity ON github_entity.installation_id::text = job_agent.config->>'installationId' WHERE github_entity.slug = job_agent.config->>'owner'; "Length of output: 436
Script:
#!/bin/bash # Find type definitions and interfaces ast-grep --pattern 'interface JobAgent { $$$ }' ast-grep --pattern 'interface GithubEntity { $$$ }' # Find other jobAgent insertions with github-app type rg -A 5 'type: "github-app",'Length of output: 980
Script:
#!/bin/bash # Search for schema definitions rg -g '*.{ts,js}' -A 10 'CREATE TABLE.*job_agent' rg -g '*.{ts,js}' -A 10 'CREATE TABLE.*github_entity' # Search for type declarations rg -g '*.{ts,js}' -A 5 'type JobAgentConfig = ' rg -g '*.{ts,js}' -A 5 'interface JobAgentConfig'Length of output: 209
packages/api/src/router/github.ts (5)
11-12: Imports updated correctly.The renamed entities
githubEntityandgithubEntityInsertare correctly imported from@ctrlplane/db/schema, reflecting the refactoring fromgithubOrganizationandgithubOrganizationInsert.
46-47: Type alias 'InstallationOctokitClient' defined appropriately.Defining
InstallationOctokitClientenhances type safety by specifying the return type ofgetOctokitInstallation, which improves code readability and maintainability.
87-89: Type 'Workflow' defined correctly for GitHub workflows.The
Workflowtype accurately represents a single workflow from the GitHub API response, ensuring precise typing for workflow-related operations.
90-94: Type 'Repo' extended with 'workflows' property correctly.The
Repotype is appropriately extended to include theworkflowsarray, combining repository data with its associated workflows for more comprehensive data handling.
354-362: Ensure 'jobAgent' is correctly configured with necessary fields.The
jobAgentinsertion seems appropriate, but verify that all required fields and configurations are present and correctly set, especially if there are additional constraints or defaults in the database schema.packages/validators/src/github/index.ts (1)
8-8: Review the necessity of both 'optional' and 'nullable' for 'ref' property.The
refproperty is defined asz.string().min(1).optional().nullable(). Using both.optional()and.nullable()might be redundant. Ifrefcan beundefinedornull, ensure that downstream code correctly handles both cases. Consider if only one is sufficient for your use case.Would you like to streamline the schema by removing one of these modifiers if appropriate?
apps/webservice/src/app/api/github/octokit.ts (1)
38-40: Exported type 'AuthedOctokitClient' enhances type safety.Defining
AuthedOctokitClientas a non-nullable return type ensures that any usage of this type is safeguarded againstnullvalues, improving robustness.apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/EntityActionDropdown.tsx (2)
16-19: LGTM! Props type definition aligns with the entity-based model.The type definition correctly uses the new
GithubEntitytype from the schema.
45-47: LGTM! Dialog integration is consistent.The GithubRemoveEntityDialog is correctly integrated with the updated prop name.
apps/webservice/src/components/form/job-agent/JobAgentConfig.tsx (3)
25-32: LGTM! Query setup is well-structured.The query is correctly configured with proper enablement condition and required parameters.
38-43: LGTM! Loading state improves user experience.Good addition of a loading spinner with descriptive text.
52-52: Fix typo in placeholder text.The placeholder text has been corrected from "Unsupport job agent" to "Unsupported job agent".
packages/db/src/schema/github.ts (3)
27-30: LGTM! Entity type enum is well-defined.The enum correctly defines the supported GitHub entity types.
58-63: LGTM! Type exports are properly defined.The types and insert schema are correctly defined with appropriate field omissions.
Line range hint
32-57: Verify database constraints for the entity table.The table definition looks good with proper foreign key constraints and unique index. However, we should verify if any existing queries or migrations handle the transition from organization to entity correctly.
✅ Verification successful
Database constraints are properly maintained ✅
The transition from
github_organizationtogithub_entityis handled correctly through migration0061_left_jocasta.sqlwith proper constraint management. The new schema maintains all necessary foreign key relationships and the unique index on installation and workspace IDs.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential migration issues # Search for direct table references rg -A 2 "github_organization" # Check for queries that might need updates rg -A 2 "FROM.*github_organization"Length of output: 75432
Script:
#!/bin/bash # Search for SQL queries and code references rg -t ts -t js -A 2 "github_organization" --glob '!packages/db/drizzle/**' # Check schema files for foreign key relationships rg -t ts "references|foreignKey" packages/db/src/schema/Length of output: 10776
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/page.tsx (1)
Line range hint
89-97: LGTM! Component update aligns with the entity model refactoring.The change from
GithubConnectedOrgstoGithubConnectedEntitiesmaintains interface compatibility while supporting the new generalized entity model.apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/GithubAddEntityDialog.tsx (1)
22-31: LGTM! Type definition properly handles entity discrimination.The updated type definition properly handles both user and organization entities with appropriate type discrimination and required fields.
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/GithubConnectedEntities.tsx (1)
39-46: LGTM! API integration properly handles the entity model.The API calls have been correctly updated to use the new entity-based endpoints while maintaining proper error handling and type safety.
packages/db/src/schema/job.ts (1)
63-63: Verify validator package update for the new job status.The addition of "successful" status to the enum requires a corresponding update in the validators package as mentioned in the comment above.
Run the following script to verify the validator package update:
✅ Verification successful
Validator package is already updated with "successful" status
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the validators package has been updated with the new status # Look for the job status validator definition rg -A 10 "jobStatus.*=.*z\.enum" packages/validators/src/jobs/Length of output: 942
packages/db/drizzle/0061_left_jocasta.sql (7)
1-5: LGTM! Well-structured ENUM type creation.The creation of
github_entity_typeENUM with proper exception handling for duplicates is a good practice.
7-8: LGTM! Good choice of naming.The rename operations effectively generalize the schema, and using
slugis a more appropriate term for the identifier.
9-11: LGTM! Proper constraint management.Dropping constraints before recreating them with updated names is the correct approach when renaming tables.
14-18: LGTM! Well-defined foreign key constraint.The foreign key constraint is properly defined with appropriate CASCADE/NO ACTION behaviors and exception handling.
20-24: LGTM! Consistent constraint definition.The workspace foreign key constraint follows the same good practices as the user constraint.
13-13: Verify data migration for existing records.The column addition looks good with appropriate default and NOT NULL constraints. However, let's verify the impact on existing records.
✅ Verification successful
Migration is correctly implemented and safe for existing records
The migration properly creates the enum type with valid values ('organization', 'user') and adds the column with appropriate constraints. The default value ensures all existing records will be properly migrated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any NULL values in the type column and verify all records have been migrated correctly echo "Checking type column in github_entity table..." psql -c "SELECT COUNT(*) as total_records, COUNT(CASE WHEN type IS NULL THEN 1 END) as null_types, COUNT(CASE WHEN type = 'organization' THEN 1 END) as org_types, COUNT(CASE WHEN type = 'user' THEN 1 END) as user_types FROM github_entity;"Length of output: 489
Script:
#!/bin/bash # Check for the creation of github_entity_type enum in previous migrations fd . "packages/db/drizzle" -e sql -x cat {} \; | grep -B 2 -A 2 "github_entity_type"Length of output: 841
26-26: Verify the removal of branch column.Please confirm that the branch information is either no longer needed or has been moved to a more appropriate location.
✅ Verification successful
Branch column removal is safe to proceed
The branch column was initially added with a default value of 'main', but there are no active references to it in the codebase. The github_entity table continues to be used throughout the application without requiring branch information.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any references to the branch column in the codebase echo "Searching for references to branch column..." rg -i "branch.*github.*organization|github.*organization.*branch" --type ts --type tsxLength of output: 215
Script:
#!/bin/bash # Search for branch-related patterns in migrations and TypeScript files echo "Checking SQL migrations for branch column..." rg -i "branch" --type sql echo -e "\nChecking TypeScript files for branch references..." rg -i "branch.*github" --type ts echo -e "\nChecking for GitHub entity type definitions..." ast-grep --pattern 'interface GithubEntity { $$$ }'Length of output: 625
Script:
#!/bin/bash # Check for any references to github_entity table in TypeScript files echo "Checking for github_entity table usage..." rg -i "github_entity" --type ts -A 3 echo -e "\nChecking for GitHub entity schema definitions..." rg -i "github.*entity.*schema" --type ts -A 5Length of output: 3004
packages/db/drizzle/meta/_journal.json (1)
432-437: LGTM! Properly formatted migration entry.The new migration entry follows the established pattern and includes all required fields.
| {githubEntities.map(({ slug, avatarUrl }) => ( | ||
| <CommandItem | ||
| key={id} | ||
| value={login} | ||
| key={slug} | ||
| value={slug} |
There was a problem hiding this comment.
Ensure unique keys in list rendering
Using slug as the key may not guarantee uniqueness across different entities. Use installationId, which is unique for each installation, to prevent potential rendering issues.
Apply this diff to fix the key:
-{githubEntities.map(({ slug, avatarUrl }) => (
+{githubEntities.map(({ installationId, slug, avatarUrl }) => (- key={slug}
+ key={installationId}📝 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.
| {githubEntities.map(({ slug, avatarUrl }) => ( | |
| <CommandItem | |
| key={id} | |
| value={login} | |
| key={slug} | |
| value={slug} | |
| {githubEntities.map(({ installationId, slug, avatarUrl }) => ( | |
| <CommandItem | |
| key={installationId} | |
| value={slug} |
Ensure unique values in CommandItem components
The value prop in CommandItem should be unique to each item. If slug might not be unique, consider using a combination of type and slug or installationId.
Apply this diff to ensure unique value props:
- value={slug}
+ value={`${type}:${slug}`}Remember to include type in the destructured properties:
-{githubEntities.map(({ installationId, slug, avatarUrl }) => (
+{githubEntities.map(({ installationId, type, slug, avatarUrl }) => (Committable suggestion skipped: line range outside the PR's diff.
| const headers = { | ||
| "X-GitHub-Api-Version": "2022-11-28", | ||
| authorization: `Bearer ${installationToken.token}`, | ||
| }; |
There was a problem hiding this comment.
Use Authorization: token instead of Bearer for GitHub authentication.
GitHub API expects the Authorization header to use the token scheme for personal access tokens and installation tokens, not Bearer. Using Bearer might result in authentication errors.
Apply this diff to correct the authorization header:
const headers = {
"X-GitHub-Api-Version": "2022-11-28",
- authorization: `Bearer ${installationToken.token}`,
+ authorization: `token ${installationToken.token}`,
};📝 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 headers = { | |
| "X-GitHub-Api-Version": "2022-11-28", | |
| authorization: `Bearer ${installationToken.token}`, | |
| }; | |
| const headers = { | |
| "X-GitHub-Api-Version": "2022-11-28", | |
| authorization: `token ${installationToken.token}`, | |
| }; |
There was a problem hiding this comment.
i'm not sure, using Bearer currently works correctly
There was a problem hiding this comment.
@adityachoudhari26 You're right - I apologize for my earlier suggestion. The GitHub API accepts both Bearer and token schemes for authentication. Since you've confirmed that Bearer works correctly in your implementation, we should keep it as is.
🔗 Analysis chain
@adityachoudhari26 Thank you for the feedback. If the current implementation with Bearer scheme is working correctly, I'll retract my suggestion. Let's keep the existing code as is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GitHub's official documentation about installation token authentication
curl -s "https://docs.github.com/en/rest/authentication/authenticating-to-the-rest-api?apiVersion=2022-11-28" | grep -A 5 -B 5 "Authorization.*token"
# Search for examples in the codebase showing GitHub API authentication
rg -A 2 -B 2 "authorization.*token.*installationToken" --type ts
Length of output: 65785
Script:
#!/bin/bash
# Search GitHub's REST API documentation for installation token authentication
curl -s "https://docs.github.com/en/rest/apps/apps?apiVersion=2022-11-28" | grep -A 5 -B 5 "Authorization.*token"
# Search for examples in GitHub's official Octokit documentation
curl -s "https://github.com/octokit/octokit.js/blob/master/README.md" | grep -A 5 -B 5 "authorization.*token"
# Search for more examples in the codebase
rg -A 2 -B 2 "authorization.*token" --type ts
Length of output: 131070
Script:
#!/bin/bash
# Search for GitHub API authentication examples in the codebase
rg -A 2 -B 2 "octokit.*auth.*installation" --type ts
# Search for GitHub's official examples
curl -s "https://raw.githubusercontent.com/octokit/octokit.js/master/README.md" | grep -A 5 -B 5 "createAppAuth"
Length of output: 801
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#288
File: apps/event-worker/src/job-dispatch/github.ts:126-129
Timestamp: 2025-01-22T07:38:08.672Z
Learning: GitHub API accepts both 'Bearer' and 'token' schemes for authentication with installation tokens.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| .get({ ...parsedConfig, headers }) | ||
| .then((r) => r.data.default_branch)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential errors when retrieving the default branch.
If the API call to get the repository details fails, the code may throw an unhandled exception. It's advisable to include error handling to manage such cases gracefully.
You can wrap the API call in a try-catch block:
- const ref =
- parsedConfig.ref ??
- (await octokit.rest.repos
- .get({ ...parsedConfig, headers })
- .then((r) => r.data.default_branch));
+ let ref = parsedConfig.ref;
+ if (!ref) {
+ try {
+ const repoData = await octokit.rest.repos.get({
+ owner: parsedConfig.owner,
+ repo: parsedConfig.repo,
+ headers,
+ });
+ ref = repoData.data.default_branch;
+ } catch (error) {
+ logger.error(`Failed to get ref for GitHub action job ${je.id}: ${error.message}`);
+ await db
+ .update(job)
+ .set({
+ status: JobStatus.InvalidJobAgent,
+ message: `Failed to get ref for GitHub action job: ${error.message}`,
+ })
+ .where(eq(job.id, je.id));
+ return;
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
| const orgData = await client.orgs.get({ | ||
| org: String(installationTargetId), | ||
| headers: { "X-GitHub-Api-Version": "2022-11-28" }, | ||
| }); |
There was a problem hiding this comment.
Use organization login name instead of ID in client.orgs.get.
The orgs.get method requires the organization's login name (org), not the ID. Passing the ID may result in an error.
Modify the code to use the organization's login name:
const orgData = await client.orgs.get({
- org: String(installationTargetId),
+ org: installation.account.login,
headers: { "X-GitHub-Api-Version": "2022-11-28" },
});Ensure that installation.account.login is available and represents the organization's login name.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
so i thought this too but it seems the api works with passing in the installation target ID here (which is just the id of the org)
There was a problem hiding this comment.
@adityachoudhari26 Thank you for the correction. I apologize for the confusion. If you've confirmed that the API works with the organization ID in this context, then the current implementation is correct.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of orgs.get in the codebase
rg -A 3 -B 3 "orgs\.get\("
# Search for any comments or documentation about GitHub organization IDs
rg -i "organization.*id.*github"
Length of output: 4903
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#288
File: apps/webservice/src/app/api/github/installation/route.ts:26-29
Timestamp: 2025-01-22T07:43:17.724Z
Learning: In the GitHub installation flow, the `orgs.get` API endpoint accepts both organization login names and organization IDs (installation target IDs) as the `org` parameter.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const createEntity = | ||
| installation.data.target_type === "Organization" | ||
| ? createOrganizationEntity | ||
| : createUserEntity; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle unrecognized target_type values.
Currently, the code only handles Organization and defaults to User for any other target_type. If target_type can have other values, it's important to handle them explicitly to avoid unexpected behavior.
Modify the code to include an explicit check:
const createEntity =
installation.data.target_type === "Organization"
? createOrganizationEntity
- : createUserEntity;
+ : installation.data.target_type === "User"
+ ? createUserEntity
+ : null;
if (createEntity == null) {
return NextResponse.json(
{ error: "Unsupported installation target type" },
{ status: BAD_REQUEST },
);
}📝 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 createEntity = | |
| installation.data.target_type === "Organization" | |
| ? createOrganizationEntity | |
| : createUserEntity; | |
| const createEntity = | |
| installation.data.target_type === "Organization" | |
| ? createOrganizationEntity | |
| : installation.data.target_type === "User" | |
| ? createUserEntity | |
| : null; | |
| if (createEntity == null) { | |
| return NextResponse.json( | |
| { error: "Unsupported installation target type" }, | |
| { status: BAD_REQUEST }, | |
| ); | |
| } |
There was a problem hiding this comment.
those are the only two types
There was a problem hiding this comment.
@adityachoudhari26 Understood, thank you for the clarification. The current implementation is correct then.
ʕ •ᴥ•ʔ
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#288
File: apps/webservice/src/app/api/github/installation/route.ts:152-155
Timestamp: 2025-01-22T07:44:33.746Z
Learning: GitHub installation target_type can only have two possible values: "Organization" and "User". No additional error handling is needed for other values.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const getMembers = async ( | ||
| page: number, | ||
| fetchedMembers: Member[], | ||
| ): Promise<Member[]> => { | ||
| const { data } = await installationOctokit.orgs.listMembers({ | ||
| org: installation.account?.login ?? "", | ||
| per_page: 100, | ||
| page, | ||
| headers: { | ||
| "X-GitHub-Api-Version": "2022-11-28", | ||
| authorization: `Bearer ${installationToken.token}`, | ||
| }, | ||
| }); | ||
| const members = [...data, ...fetchedMembers]; | ||
| if (data.length < 100) return members; | ||
| return getMembers(page + 1, members); | ||
| }; | ||
|
|
||
| const members = await getMembers(1, []); | ||
| const isUserInGithubOrg = | ||
| members.find((m) => m.id === input.githubUserId) != null; | ||
| if (!isUserInGithubOrg) return null; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate manual 'authorization' header in 'getMembers' function.
In the getMembers function, manually setting the 'authorization' header is unnecessary since installationOctokit handles authentication. Removing it simplifies the code and relies on the client's internal mechanisms.
Apply this diff:
headers: {
"X-GitHub-Api-Version": "2022-11-28",
- authorization: `Bearer ${installationToken.token}`,
},📝 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 getMembers = async ( | |
| page: number, | |
| fetchedMembers: Member[], | |
| ): Promise<Member[]> => { | |
| const { data } = await installationOctokit.orgs.listMembers({ | |
| org: installation.account?.login ?? "", | |
| per_page: 100, | |
| page, | |
| headers: { | |
| "X-GitHub-Api-Version": "2022-11-28", | |
| authorization: `Bearer ${installationToken.token}`, | |
| }, | |
| }); | |
| const members = [...data, ...fetchedMembers]; | |
| if (data.length < 100) return members; | |
| return getMembers(page + 1, members); | |
| }; | |
| const members = await getMembers(1, []); | |
| const isUserInGithubOrg = | |
| members.find((m) => m.id === input.githubUserId) != null; | |
| if (!isUserInGithubOrg) return null; | |
| const getMembers = async ( | |
| page: number, | |
| fetchedMembers: Member[], | |
| ): Promise<Member[]> => { | |
| const { data } = await installationOctokit.orgs.listMembers({ | |
| org: installation.account?.login ?? "", | |
| per_page: 100, | |
| page, | |
| headers: { | |
| "X-GitHub-Api-Version": "2022-11-28", | |
| }, | |
| }); | |
| const members = [...data, ...fetchedMembers]; | |
| if (data.length < 100) return members; | |
| return getMembers(page + 1, members); | |
| }; | |
| const members = await getMembers(1, []); | |
| const isUserInGithubOrg = | |
| members.find((m) => m.id === input.githubUserId) != null; | |
| if (!isUserInGithubOrg) return null; |
packages/api/src/router/github.ts
Outdated
| }) | ||
| .then(async ({ data: installation }) => { | ||
| const installationOctokit = getOctokitInstallation(installation.id); | ||
| const installationToken = (await installationOctokit.auth({ | ||
| type: "installation", | ||
| installationId: installation.id, | ||
| })) as { token: string }; | ||
|
|
||
| type Repo = | ||
| RestEndpointMethodTypes["repos"]["listForOrg"]["response"]["data"][number]; | ||
| const repos: Repo[] = []; | ||
|
|
||
| const getRepos = async (page: number) => { | ||
| const { data } = await installationOctokit.repos.listForOrg({ | ||
| org: input.owner, | ||
| per_page: 100, | ||
| page, | ||
| headers: { | ||
| "X-GitHub-Api-Version": "2022-11-28", | ||
| authorization: `Bearer ${installationToken.token}`, | ||
| }, | ||
| }); | ||
| repos.push(...data); | ||
| if (data.length < 100) return; | ||
| return getRepos(page + 1); | ||
| }; | ||
| })) ?? {}; | ||
|
|
||
| await getRepos(1); | ||
| return repos; | ||
| }), | ||
| ), | ||
| if (installation == null) | ||
| throw new TRPCError({ | ||
| code: "NOT_FOUND", | ||
| message: "Installation not found", | ||
| }); | ||
|
|
||
| workflows: createTRPCRouter({ | ||
| list: protectedProcedure | ||
| .meta({ | ||
| authorizationCheck: ({ canUser, input }) => | ||
| canUser.perform(Permission.WorkspaceListIntegrations).on({ | ||
| type: "workspace", | ||
| id: input.workspaceId, | ||
| }), | ||
| }) | ||
| .input( | ||
| z.object({ | ||
| installationId: z.number(), | ||
| owner: z.string(), | ||
| repo: z.string(), | ||
| workspaceId: z.string().uuid(), | ||
| }), | ||
| ) | ||
| .query(async ({ input }) => { | ||
| const installationOctokit = getOctokitInstallation( | ||
| input.installationId, | ||
| ); | ||
| const installationOctokit = getOctokitInstallation(installation.id); | ||
| const installationToken = (await installationOctokit.auth({ | ||
| type: "installation", | ||
| installationId: installation.id, | ||
| })) as { token: string }; | ||
|
|
||
| const installationToken = (await installationOctokit.auth({ | ||
| type: "installation", | ||
| installationId: input.installationId, | ||
| })) as { token: string }; | ||
|
|
||
| const workflows: RestEndpointMethodTypes["actions"]["listRepoWorkflows"]["response"]["data"]["workflows"] = | ||
| []; | ||
|
|
||
| const getWorkflows = async (page: number) => { | ||
| const { data } = await installationOctokit.actions.listRepoWorkflows({ | ||
| ...input, | ||
| page, | ||
| per_page: 100, | ||
| headers: { | ||
| "X-GitHub-Api-Version": "2022-11-28", | ||
| authorization: `Bearer ${installationToken.token}`, | ||
| }, | ||
| }); | ||
| workflows.push(...data.workflows); | ||
| if (data.workflows.length < 100) return; | ||
| return getWorkflows(page + 1); | ||
| }; | ||
|
|
||
| await getWorkflows(1); | ||
| return workflows; | ||
| }), | ||
| }), | ||
| return getRepos( | ||
| 1, | ||
| [], | ||
| installationOctokit, | ||
| installationToken, | ||
| input.owner, | ||
| ); | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use 'getOctokit()' to ensure the GitHub bot is configured before API calls.
Directly accessing octokit can lead to null reference errors if the GitHub bot is not configured. The getOctokit() function includes a check and throws an error if the bot is not set up, ensuring safer execution.
Apply this diff to use getOctokit():
- (await octokit?.apps.getInstallation({
+ (await getOctokit().apps.getInstallation({
installation_id: input.installationId,
})) ?? {};📝 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.
| .query(async ({ input }) => { | |
| const { data: installation } = | |
| (await octokit?.apps.getInstallation({ | |
| installation_id: input.installationId, | |
| }) | |
| .then(async ({ data: installation }) => { | |
| const installationOctokit = getOctokitInstallation(installation.id); | |
| const installationToken = (await installationOctokit.auth({ | |
| type: "installation", | |
| installationId: installation.id, | |
| })) as { token: string }; | |
| type Repo = | |
| RestEndpointMethodTypes["repos"]["listForOrg"]["response"]["data"][number]; | |
| const repos: Repo[] = []; | |
| const getRepos = async (page: number) => { | |
| const { data } = await installationOctokit.repos.listForOrg({ | |
| org: input.owner, | |
| per_page: 100, | |
| page, | |
| headers: { | |
| "X-GitHub-Api-Version": "2022-11-28", | |
| authorization: `Bearer ${installationToken.token}`, | |
| }, | |
| }); | |
| repos.push(...data); | |
| if (data.length < 100) return; | |
| return getRepos(page + 1); | |
| }; | |
| })) ?? {}; | |
| await getRepos(1); | |
| return repos; | |
| }), | |
| ), | |
| if (installation == null) | |
| throw new TRPCError({ | |
| code: "NOT_FOUND", | |
| message: "Installation not found", | |
| }); | |
| workflows: createTRPCRouter({ | |
| list: protectedProcedure | |
| .meta({ | |
| authorizationCheck: ({ canUser, input }) => | |
| canUser.perform(Permission.WorkspaceListIntegrations).on({ | |
| type: "workspace", | |
| id: input.workspaceId, | |
| }), | |
| }) | |
| .input( | |
| z.object({ | |
| installationId: z.number(), | |
| owner: z.string(), | |
| repo: z.string(), | |
| workspaceId: z.string().uuid(), | |
| }), | |
| ) | |
| .query(async ({ input }) => { | |
| const installationOctokit = getOctokitInstallation( | |
| input.installationId, | |
| ); | |
| const installationOctokit = getOctokitInstallation(installation.id); | |
| const installationToken = (await installationOctokit.auth({ | |
| type: "installation", | |
| installationId: installation.id, | |
| })) as { token: string }; | |
| const installationToken = (await installationOctokit.auth({ | |
| type: "installation", | |
| installationId: input.installationId, | |
| })) as { token: string }; | |
| const workflows: RestEndpointMethodTypes["actions"]["listRepoWorkflows"]["response"]["data"]["workflows"] = | |
| []; | |
| const getWorkflows = async (page: number) => { | |
| const { data } = await installationOctokit.actions.listRepoWorkflows({ | |
| ...input, | |
| page, | |
| per_page: 100, | |
| headers: { | |
| "X-GitHub-Api-Version": "2022-11-28", | |
| authorization: `Bearer ${installationToken.token}`, | |
| }, | |
| }); | |
| workflows.push(...data.workflows); | |
| if (data.workflows.length < 100) return; | |
| return getWorkflows(page + 1); | |
| }; | |
| await getWorkflows(1); | |
| return workflows; | |
| }), | |
| }), | |
| return getRepos( | |
| 1, | |
| [], | |
| installationOctokit, | |
| installationToken, | |
| input.owner, | |
| ); | |
| }), | |
| .query(async ({ input }) => { | |
| const { data: installation } = | |
| (await getOctokit().apps.getInstallation({ | |
| installation_id: input.installationId, | |
| })) ?? {}; | |
| if (installation == null) | |
| throw new TRPCError({ | |
| code: "NOT_FOUND", | |
| message: "Installation not found", | |
| }); | |
| const installationOctokit = getOctokitInstallation(installation.id); | |
| const installationToken = (await installationOctokit.auth({ | |
| type: "installation", | |
| installationId: installation.id, | |
| })) as { token: string }; | |
| return getRepos( | |
| 1, | |
| [], | |
| installationOctokit, | |
| installationToken, | |
| input.owner, | |
| ); | |
| }), |
| const getRepos = async ( | ||
| page: number, | ||
| fetchedRepos: Repo[], | ||
| installationOctokit: InstallationOctokitClient, | ||
| installationToken: { token: string }, | ||
| owner: string, | ||
| ) => { | ||
| const { data } = await installationOctokit.repos.listForOrg({ | ||
| org: owner, | ||
| per_page: 100, | ||
| page, | ||
| headers: { | ||
| "X-GitHub-Api-Version": "2022-11-28", | ||
| authorization: `Bearer ${installationToken.token}`, | ||
| }, | ||
| }); | ||
|
|
||
| const reposWithWorkflows = await Promise.all( | ||
| data.map(async (repo) => { | ||
| const workflows = await getWorkflows( | ||
| 1, | ||
| [], | ||
| repo, | ||
| installationOctokit, | ||
| installationToken, | ||
| ); | ||
| return { ...repo, workflows }; | ||
| }), | ||
| ); | ||
|
|
||
| const repos = [...reposWithWorkflows, ...fetchedRepos]; | ||
| if (reposWithWorkflows.length < 100) return repos; | ||
| return getRepos( | ||
| page + 1, | ||
| repos, | ||
| installationOctokit, | ||
| installationToken, | ||
| owner, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant 'authorization' header in 'getRepos' function.
Similar to the previous point, the installationOctokit client in getRepos is already authenticated. The manual addition of the 'authorization' header is unnecessary and should be omitted to prevent potential authentication issues.
Apply this diff to correct the headers:
headers: {
"X-GitHub-Api-Version": "2022-11-28",
- authorization: `Bearer ${installationToken.token}`,
},📝 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 getRepos = async ( | |
| page: number, | |
| fetchedRepos: Repo[], | |
| installationOctokit: InstallationOctokitClient, | |
| installationToken: { token: string }, | |
| owner: string, | |
| ) => { | |
| const { data } = await installationOctokit.repos.listForOrg({ | |
| org: owner, | |
| per_page: 100, | |
| page, | |
| headers: { | |
| "X-GitHub-Api-Version": "2022-11-28", | |
| authorization: `Bearer ${installationToken.token}`, | |
| }, | |
| }); | |
| const reposWithWorkflows = await Promise.all( | |
| data.map(async (repo) => { | |
| const workflows = await getWorkflows( | |
| 1, | |
| [], | |
| repo, | |
| installationOctokit, | |
| installationToken, | |
| ); | |
| return { ...repo, workflows }; | |
| }), | |
| ); | |
| const repos = [...reposWithWorkflows, ...fetchedRepos]; | |
| if (reposWithWorkflows.length < 100) return repos; | |
| return getRepos( | |
| page + 1, | |
| repos, | |
| installationOctokit, | |
| installationToken, | |
| owner, | |
| ); | |
| }; | |
| const getRepos = async ( | |
| page: number, | |
| fetchedRepos: Repo[], | |
| installationOctokit: InstallationOctokitClient, | |
| installationToken: { token: string }, | |
| owner: string, | |
| ) => { | |
| const { data } = await installationOctokit.repos.listForOrg({ | |
| org: owner, | |
| per_page: 100, | |
| page, | |
| headers: { | |
| "X-GitHub-Api-Version": "2022-11-28", | |
| }, | |
| }); | |
| const reposWithWorkflows = await Promise.all( | |
| data.map(async (repo) => { | |
| const workflows = await getWorkflows( | |
| 1, | |
| [], | |
| repo, | |
| installationOctokit, | |
| installationToken, | |
| ); | |
| return { ...repo, workflows }; | |
| }), | |
| ); | |
| const repos = [...reposWithWorkflows, ...fetchedRepos]; | |
| if (reposWithWorkflows.length < 100) return repos; | |
| return getRepos( | |
| page + 1, | |
| repos, | |
| installationOctokit, | |
| installationToken, | |
| owner, | |
| ); | |
| }; |
| const getWorkflows = async ( | ||
| page: number, | ||
| fetchedWorkflows: Workflow[], | ||
| repo: RestEndpointMethodTypes["repos"]["listForOrg"]["response"]["data"][number], | ||
| installationOctokit: InstallationOctokitClient, | ||
| installationToken: { token: string }, | ||
| ) => { | ||
| const { data } = await installationOctokit.actions.listRepoWorkflows({ | ||
| repo: repo.name, | ||
| owner: repo.owner.login, | ||
| page, | ||
| per_page: 100, | ||
| headers: { | ||
| "X-GitHub-Api-Version": "2022-11-28", | ||
| authorization: `Bearer ${installationToken.token}`, | ||
| }, | ||
| }); | ||
| const workflows = [...data.workflows, ...fetchedWorkflows]; | ||
| if (data.workflows.length < 100) return workflows; | ||
| return getWorkflows( | ||
| page + 1, | ||
| workflows, | ||
| repo, | ||
| installationOctokit, | ||
| installationToken, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid manually setting the 'authorization' header in 'getWorkflows' function.
In the getWorkflows function, the installationOctokit client is already authenticated. Manually setting the 'authorization' header is unnecessary and could interfere with the client's authentication handling. The Octokit library manages authentication internally when the client is properly configured.
Apply this diff to remove the redundant header:
headers: {
"X-GitHub-Api-Version": "2022-11-28",
- authorization: `Bearer ${installationToken.token}`,
},📝 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 getWorkflows = async ( | |
| page: number, | |
| fetchedWorkflows: Workflow[], | |
| repo: RestEndpointMethodTypes["repos"]["listForOrg"]["response"]["data"][number], | |
| installationOctokit: InstallationOctokitClient, | |
| installationToken: { token: string }, | |
| ) => { | |
| const { data } = await installationOctokit.actions.listRepoWorkflows({ | |
| repo: repo.name, | |
| owner: repo.owner.login, | |
| page, | |
| per_page: 100, | |
| headers: { | |
| "X-GitHub-Api-Version": "2022-11-28", | |
| authorization: `Bearer ${installationToken.token}`, | |
| }, | |
| }); | |
| const workflows = [...data.workflows, ...fetchedWorkflows]; | |
| if (data.workflows.length < 100) return workflows; | |
| return getWorkflows( | |
| page + 1, | |
| workflows, | |
| repo, | |
| installationOctokit, | |
| installationToken, | |
| ); | |
| }; | |
| const getWorkflows = async ( | |
| page: number, | |
| fetchedWorkflows: Workflow[], | |
| repo: RestEndpointMethodTypes["repos"]["listForOrg"]["response"]["data"][number], | |
| installationOctokit: InstallationOctokitClient, | |
| installationToken: { token: string }, | |
| ) => { | |
| const { data } = await installationOctokit.actions.listRepoWorkflows({ | |
| repo: repo.name, | |
| owner: repo.owner.login, | |
| page, | |
| per_page: 100, | |
| headers: { | |
| "X-GitHub-Api-Version": "2022-11-28", | |
| }, | |
| }); | |
| const workflows = [...data.workflows, ...fetchedWorkflows]; | |
| if (data.workflows.length < 100) return workflows; | |
| return getWorkflows( | |
| page + 1, | |
| workflows, | |
| repo, | |
| installationOctokit, | |
| installationToken, | |
| ); | |
| }; |
There was a problem hiding this comment.
not sure I understand. I am setting the token per request, each request is independent no?
There was a problem hiding this comment.
@adityachoudhari26 I understand your concern about independent requests. However, when you create the installationOctokit instance using getOctokitInstallation(), it's already configured with the installation authentication strategy (createAppAuth). The Octokit client automatically:
- Manages the token lifecycle
- Adds the authorization header to each request
- Handles token refresh when needed
So when you call installationOctokit.actions.listRepoWorkflows(), it will automatically authenticate the request. Setting the token manually in headers is redundant and could potentially interfere with Octokit's internal token management.
🤔
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#288
File: packages/api/src/router/github.ts:95-121
Timestamp: 2025-01-22T07:46:21.457Z
Learning: When using Octokit client initialized with createAppAuth strategy, avoid manually setting authorization headers as the client automatically handles authentication for each request.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| @@ -37,7 +33,7 @@ export const OrgActionDropdown: React.FC<OrgActionDropdownProps> = ({ | |||
| </DropdownMenuTrigger> | |||
| <DropdownMenuContent> | |||
| <Link | |||
| href={`${githubConfig.url}/organizations/${org.organizationName}/settings/installations/${org.installationId}`} | |||
| href={`${githubConfig.url}/organizations/${entity.slug}/settings/installations/${entity.installationId}`} | |||
There was a problem hiding this comment.
💡 Codebase verification
URL pattern needs to be adjusted for user installations
The current URL pattern only works for organization entities but not for user entities. Based on GitHub's URL structure:
- For organizations:
/organizations/${entity.slug}/settings/installations/${entity.installationId} - For users:
/${entity.slug}/settings/installations/${entity.installationId}
The URL construction should be conditional based on entity.type.
🔗 Analysis chain
Verify the URL pattern for GitHub settings.
Please ensure that the URL pattern ${githubConfig.url}/organizations/${entity.slug}/settings/installations/${entity.installationId} works for both organization and user entities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the URL pattern is valid for both organization and user entities
# Search for any similar URL patterns in the codebase
rg -A 2 "organizations/.*?/settings/installations"
# Check GitHub API documentation or similar URL constructions
curl -s https://api.github.com/meta | jq '.installations_url'
Length of output: 651
Script:
#!/bin/bash
# Search for installationId usage and type definitions
echo "=== Searching for installationId usage ==="
rg -A 3 "installationId"
echo -e "\n=== Looking for entity type definitions ==="
ast-grep --pattern 'interface $_ {
$$$
installationId: $_
$$$
}'
echo -e "\n=== Searching for GitHub URL construction ==="
rg -A 2 "githubConfig.url"
Length of output: 12752
| </Popover> | ||
|
|
||
| <Input | ||
| placeholder="Ref (uses default branch if not set)" |
There was a problem hiding this comment.
| placeholder="Ref (uses default branch if not set)" | |
| placeholder="Git reference (uses repositories default if not set)" |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/SelectPreconnectedEntityDialogContent.tsx (1)
Line range hint
41-54: Consider enhancing error handling in the save handler.While the null checks are good, consider adding error handling to provide feedback to the user when the mutation fails.
const handleSave = async () => { if (value == null) return; const entity = githubEntities.find((e) => e.slug === value); if (entity == null) return; - await githubEntityCreate.mutateAsync({ - ...entity, - workspaceId, - }); + try { + await githubEntityCreate.mutateAsync({ + ...entity, + workspaceId, + }); + onSave(); + router.refresh(); + utils.job.agent.byWorkspaceId.invalidate(workspaceId); + } catch (error) { + // Handle error appropriately (e.g., show toast notification) + console.error('Failed to create GitHub entity:', error); + } - onSave(); - router.refresh(); - utils.job.agent.byWorkspaceId.invalidate(workspaceId); };apps/event-worker/src/job-dispatch/github.ts (1)
131-141: Consider adding retry logic for ref retrieval.The current implementation handles errors but doesn't attempt to retry the operation. GitHub API calls can sometimes fail transiently.
Consider implementing exponential backoff:
- const ref = - parsedConfig.ref ?? - (await octokit.rest.repos - .get({ ...parsedConfig, headers }) - .then((r) => r.data.default_branch) - .catch((e) => { - logger.error(`Failed to get ref for github action job ${je.id}`, { - error: e, - }); - return null; - })); + const getRefWithRetry = async (attempt = 1, maxAttempts = 3) => { + try { + const response = await octokit.rest.repos.get({ ...parsedConfig, headers }); + return response.data.default_branch; + } catch (e) { + if (attempt === maxAttempts) { + logger.error(`Failed to get ref for github action job ${je.id}`, { + error: e, + attempt, + }); + return null; + } + await new Promise(resolve => setTimeout(resolve, Math.pow(2, attempt) * 1000)); + return getRefWithRetry(attempt + 1, maxAttempts); + } + }; + + const ref = parsedConfig.ref ?? await getRefWithRetry();packages/api/src/router/github.ts (2)
95-121: Consider implementing cursor-based pagination.The current implementation uses page-based pagination which can lead to missing or duplicate items if the underlying data changes between requests.
Consider using cursor-based pagination if supported by the GitHub API:
const getWorkflows = async ( cursor: string | null, fetchedWorkflows: Workflow[], repo: Repo, installationOctokit: InstallationOctokitClient, installationToken: { token: string }, ) => { const { data } = await installationOctokit.actions.listRepoWorkflows({ repo: repo.name, owner: repo.owner.login, per_page: 100, after: cursor, headers: { "X-GitHub-Api-Version": "2022-11-28", authorization: `Bearer ${installationToken.token}`, }, }); const workflows = [...fetchedWorkflows, ...data.workflows]; if (!data.pagination?.hasNextPage) return workflows; return getWorkflows( data.pagination.endCursor, workflows, repo, installationOctokit, installationToken, ); };
236-252: Consider implementing rate limiting protection.The recursive member fetching could hit GitHub API rate limits. Consider implementing rate limit checking and backoff.
Add rate limit protection:
const getMembers = async ( page: number, fetchedMembers: Member[], ): Promise<Member[]> => { const { headers } = await installationOctokit.rateLimit.get(); const remaining = parseInt(headers['x-ratelimit-remaining'] ?? '0'); if (remaining < 10) { const resetTime = parseInt(headers['x-ratelimit-reset'] ?? '0') * 1000; const waitTime = resetTime - Date.now(); if (waitTime > 0) { await new Promise(resolve => setTimeout(resolve, waitTime)); } } const { data } = await installationOctokit.orgs.listMembers({ org: installation.account?.login ?? "", per_page: 100, page, headers: { "X-GitHub-Api-Version": "2022-11-28", authorization: `Bearer ${installationToken.token}`, }, }); const members = [...data, ...fetchedMembers]; if (data.length < 100) return members; return getMembers(page + 1, members); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/event-worker/src/job-dispatch/github.ts(4 hunks)apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/EntityActionDropdown.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/SelectPreconnectedEntityDialogContent.tsx(3 hunks)apps/webservice/src/components/form/job-agent/JobAgentGitHubConfig.tsx(2 hunks)packages/api/src/router/github.ts(8 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/drizzle/meta/_journal.json
🧰 Additional context used
📓 Path-based instructions (5)
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/EntityActionDropdown.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/components/form/job-agent/JobAgentGitHubConfig.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/SelectPreconnectedEntityDialogContent.tsx (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/event-worker/src/job-dispatch/github.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/api/src/router/github.ts (1)
Pattern **/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (10)
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/SelectPreconnectedEntityDialogContent.tsx (4)
19-30: LGTM! Well-structured type definition.The
PreconnectedEntityComboboxPropstype is well-defined with proper typing for GitHub entities, including a clear discriminator field for entity types.
32-34: LGTM! Clean component declaration.The component declaration follows React best practices with proper type annotations.
90-106: Previous key uniqueness concerns have been addressed.The implementation now uses
installationIdas the key, which ensures uniqueness across different entities.
124-124: LGTM! Proper button state management.The button's disabled state correctly handles both the unselected state and pending mutation state.
apps/webservice/src/app/[workspaceSlug]/settings/workspace/integrations/(integration)/github/EntityActionDropdown.tsx (2)
16-19: LGTM! Props type definition is well-structured.The type definition clearly specifies the required props and uses appropriate types from the schema.
25-29: LGTM! URL construction handles both organization and user entities.The conditional URL construction correctly handles both organization and user installation URLs based on the entity type.
apps/webservice/src/components/form/job-agent/JobAgentGitHubConfig.tsx (3)
34-70: LGTM! Repository selection UI is well-implemented.The implementation includes:
- Proper accessibility attributes
- Clear visual feedback
- Efficient state management
72-110: LGTM! Workflow selection UI follows consistent patterns.The workflow selection follows the same pattern as repository selection, maintaining consistency in the UI.
112-117: LGTM! Git reference input is properly implemented.The input field for git reference includes:
- Clear placeholder text
- Proper value handling
- Appropriate width constraint
apps/event-worker/src/job-dispatch/github.ts (1)
Line range hint
21-69: LGTM! Well-structured helper function for entity retrieval.The
getGithubEntityfunction efficiently:
- Handles both release and runbook job triggers
- Uses Promise.all for parallel execution
- Returns the first matching entity
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Chores