fix(deployment): scope GitHub repo list to app installations#2510
fix(deployment): scope GitHub repo list to app installations#2510ygrishajev merged 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughAccepts GitHubService options, makes API clients and options private, changes repo fetching to list installations and paginate per-installation repositories with deduplication, updates DI to pass new options, and adjusts several UI hooks/components and server-side props flow. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GitHubService
participant GitHubAPI as "GitHub API"
participant InternalAPI as "Internal API"
Client->>GitHubService: fetchRepos()
GitHubService->>GitHubAPI: GET /user/installations
GitHubAPI-->>GitHubService: installations[]
alt no installations
GitHubService-->>Client: []
else
loop for each installation
GitHubService->>GitHubAPI: GET /user/installations/{installationId}/repositories?per_page=150&page=N
GitHubAPI-->>GitHubService: repositories_page[]
GitHubService->>GitHubService: accumulate pages (`#fetchAllReposForInstallation`)
end
GitHubService->>GitHubService: flatten + dedupe repos by id
GitHubService-->>Client: deduplicated repos[]
end
Client->>GitHubService: fetchAccessToken(code)
GitHubService->>InternalAPI: POST /exchange-token (uses `#internalApiService`)
InternalAPI-->>GitHubService: access_token
GitHubService-->>Client: access_token
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-06-05T21:07:51.985ZApplied to files:
📚 Learning: 2025-06-19T16:00:05.428ZApplied to files:
🧬 Code graph analysis (3)apps/deploy-web/src/services/remote-deploy/github-http.service.ts (4)
apps/deploy-web/src/components/remote-deploy/Repos.tsx (2)
apps/deploy-web/src/services/app-di-container/browser-di-container.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (10)
✏️ Tip: You can disable this entire section by setting Comment |
ygrishajev
force-pushed
the
fix/deployment
branch
from
5e86305 to
8bd84d7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/deploy-web/src/services/remote-deploy/github-http.service.ts`:
- Around line 62-71: The current fetch for installation repositories uses
per_page=150, lacks pagination, and uses Promise.all which fails fast; change
the query to per_page=100, implement paginated fetching for each installation by
following the Link header (or next page URL) returned by githubApiService.get
until no next link, aggregating all response.data.repositories, and replace
Promise.all over installations.installations with Promise.allSettled so you
collect successful results and ignore/record failures; update the block that
builds repos (referencing installations.installations, the githubApiService.get
call, and the repos variable) to accumulate repositories per installation from
all pages and return the flattened successful lists while logging or capturing
errors from rejected promises.
🧹 Nitpick comments (1)
apps/deploy-web/src/services/remote-deploy/github-http.service.ts (1)
51-59: Consider the case when token is null/undefined.The
tokenparameter is optional (token?: string | null), but whennullorundefined, theAuthorization: Bearer ${token}header will send"Bearer null"or"Bearer undefined", which will fail authentication. This pattern is consistent with other methods in the class, but worth noting.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/deploy-web/src/services/remote-deploy/github-http.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.
Files:
apps/deploy-web/src/services/remote-deploy/github-http.service.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
🧬 Code graph analysis (1)
apps/deploy-web/src/services/remote-deploy/github-http.service.ts (1)
apps/deploy-web/src/types/remotedeploy.ts (1)
GithubRepository(2-2)
🔇 Additional comments (2)
apps/deploy-web/src/services/remote-deploy/github-http.service.ts (2)
73-78: Deduplication logic looks correct.Using a
Mapkeyed byrepo.idis an effective way to deduplicate repositories that may appear across multiple installations.
11-16: > Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
apps/deploy-web/src/services/remote-deploy/github-http.service.ts
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (6.09%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2510 +/- ##
==========================================
- Coverage 50.86% 50.48% -0.39%
==========================================
Files 1063 1053 -10
Lines 29432 29127 -305
Branches 6497 6467 -30
==========================================
- Hits 14972 14705 -267
+ Misses 14056 14018 -38
Partials 404 404
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
ygrishajev
force-pushed
the
fix/deployment
branch
from
8bd84d7 to
a773519
Compare
ygrishajev
force-pushed
the
fix/deployment
branch
2 times, most recently
from
72fd4ad to
9860f0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/deploy-web/src/services/remote-deploy/github-http.service.ts`:
- Around line 93-116: In `#fetchAllReposForInstallation` add a safeguard to avoid
an infinite loop when the API reports total_count > 0 but returns empty page
results: after extracting pageRepos, if pageRepos.length === 0 then break the
loop (or throw a descriptive error) to stop pagination; optionally also add a
max-iterations safety using page or perPage to cap the loop. Update the loop
logic to use the new guard so repositories, totalCount, pageRepos, perPage and
page are consistent and the function always terminates.
🧹 Nitpick comments (1)
apps/deploy-web/src/services/remote-deploy/github-http.service.ts (1)
47-53: URL construction may break ifredirectUrlcontains existing query parameters.Line 52 appends
?step=edit-deployment&type=githubdirectly. IfredirectUrlalready includes query parameters, this would produce an invalid URL (e.g.,...?existing=param?step=...).Consider using URL API or a conditional separator:
♻️ Suggested fix
reLoginWithGithub() { if (!this.#options.githubClientId) { throw new Error("GitHub client ID is required to re-login with GitHub"); } - window.location.href = `https://github.com/login/oauth/authorize?client_id=${this.#options.githubClientId}&redirect_uri=${this.#options.redirectUrl}?step=edit-deployment&type=github`; + const separator = this.#options.redirectUrl.includes("?") ? "&" : "?"; + const redirectUri = `${this.#options.redirectUrl}${separator}step=edit-deployment&type=github`; + window.location.href = `https://github.com/login/oauth/authorize?client_id=${this.#options.githubClientId}&redirect_uri=${encodeURIComponent(redirectUri)}`; }Note: The
redirect_uriparameter value should also be URL-encoded to handle special characters safely.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/deploy-web/src/components/new-deployment/NewDeploymentPage/createServerSideProps.tsapps/deploy-web/src/services/app-di-container/browser-di-container.tsapps/deploy-web/src/services/remote-deploy/github-http.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.
Files:
apps/deploy-web/src/components/new-deployment/NewDeploymentPage/createServerSideProps.tsapps/deploy-web/src/services/app-di-container/browser-di-container.tsapps/deploy-web/src/services/remote-deploy/github-http.service.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
🧬 Code graph analysis (2)
apps/deploy-web/src/components/new-deployment/NewDeploymentPage/createServerSideProps.ts (1)
apps/deploy-web/src/config/remote-deploy.config.ts (1)
CI_CD_TEMPLATE_ID(3-3)
apps/deploy-web/src/services/app-di-container/browser-di-container.ts (1)
apps/deploy-web/src/services/remote-deploy/github-http.service.ts (1)
GitHubService(24-158)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (4)
apps/deploy-web/src/components/new-deployment/NewDeploymentPage/createServerSideProps.ts (1)
21-34: Verify: fetched template is discarded when onlyrepoUrlis provided.When only
repoUrlis provided (notemplateId):
- Template is fetched using
CI_CD_TEMPLATE_ID(Line 25)- However, Line 30's condition
template && query.templateIdis falsy sincequery.templateIdisundefined- Line 34 returns
{ templateId: undefined }, discarding the fetched templateIf the intent is to always return the template when
repoUrltriggers the CI/CD flow, the condition on Line 30 may need adjustment. Otherwise, thefindByIdcall is unnecessary overhead when onlyrepoUrlis provided.apps/deploy-web/src/services/remote-deploy/github-http.service.ts (2)
64-91: Good use ofPromise.allSettledfor resilience.The implementation correctly handles partial failures when fetching repositories from multiple installations, and the deduplication logic using
Mapbyrepo.idis sound. This addresses the concerns from the previous review.
118-157: LGTM!The remaining API methods consistently use the private
#githubApiServiceclient with proper authorization headers. ThefetchAccessTokenmethod correctly uses#internalApiServicefor internal API communication.apps/deploy-web/src/services/app-di-container/browser-di-container.ts (1)
31-36: LGTM!The
GitHubServicefactory correctly passes the required options object matching theGitHubServiceOptionstype. Configuration values are properly sourced frompublicConfig.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
ygrishajev
force-pushed
the
fix/deployment
branch
2 times, most recently
from
e348660 to
7ee1527
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/deploy-web/src/components/new-deployment/NewDeploymentPage/createServerSideProps.ts (1)
21-34: Return the fetched CI/CD template whenrepoUrlis provided without explicittemplateId.Lines 21-34 have a logic flaw: when a user provides only
repoUrl(withouttemplateId), the code fetches the CI/CD template as a fallback on line 25, but then fails to return it because the condition on line 30 requires bothtemplateto exist andquery.templateIdto be explicitly set. This causes line 34 to return{ templateId: undefined }, leaving the component without a template to work with.While the deploy button flow mitigates this by always including
templateId: CI_CD_TEMPLATE_IDalongsiderepoUrl, manually constructed URLs with onlyrepoUrlwill fail silently. Consider:
- Return the template object when successfully fetched
- Set
templateIdtoCI_CD_TEMPLATE_IDin the response when it was used as the fallback
🧹 Nitpick comments (1)
apps/deploy-web/src/services/remote-deploy/github-http.service.ts (1)
81-88: Consider logging failed installation fetches for observability.When
Promise.allSettledreturns rejected results, they are silently ignored. While this is acceptable for resilience, logging failures would help with debugging and monitoring.♻️ Optional: Add logging for failed fetches
const uniqueRepos = new Map<number, GithubRepository>(); repoResults.forEach(result => { if (result.status === "fulfilled") { result.value.forEach(repo => { uniqueRepos.set(repo.id, repo); }); + } else { + console.warn("Failed to fetch repositories for installation:", result.reason); } });Note: If a logging service is available in the codebase, prefer using that over
console.warn.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/deploy-web/src/components/new-deployment/NewDeploymentPage/createServerSideProps.tsapps/deploy-web/src/services/app-di-container/browser-di-container.tsapps/deploy-web/src/services/remote-deploy/github-http.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.
Files:
apps/deploy-web/src/services/app-di-container/browser-di-container.tsapps/deploy-web/src/components/new-deployment/NewDeploymentPage/createServerSideProps.tsapps/deploy-web/src/services/remote-deploy/github-http.service.ts
🧬 Code graph analysis (1)
apps/deploy-web/src/components/new-deployment/NewDeploymentPage/createServerSideProps.ts (1)
apps/deploy-web/src/config/remote-deploy.config.ts (1)
CI_CD_TEMPLATE_ID(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: validate (apps/deploy-web) / validate-unsafe
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
apps/deploy-web/src/services/remote-deploy/github-http.service.ts (4)
1-22: LGTM! Well-structured type definitions.The new types
GitHubInstallationsResponse,GitHubInstallationReposResponse, andGitHubServiceOptionsare properly defined with appropriate typing. Good use ofcomponents["schemas"]["installation"]from@octokit/openapi-typesfor type safety.
24-41: LGTM! Clean constructor refactor with proper encapsulation.Good use of private class fields (
#) for encapsulation. The constructor properly initializes all dependencies and the options pattern allows for flexible configuration injection.
64-91: LGTM! Robust implementation with proper error handling and deduplication.Good improvements over previous implementation:
Promise.allSettledensures partial failures don't break the entire operation- Deduplication via
Map<number, GithubRepository>handles repos accessible through multiple installations- Early return for empty installations avoids unnecessary API calls
93-124: LGTM! Pagination logic with proper safeguards.The implementation addresses previous review concerns:
per_page=100respects GitHub API limits- Empty page guard (
pageRepos.length === 0) prevents infinite loopsmaxPagescalculation provides additional termination safetyapps/deploy-web/src/services/app-di-container/browser-di-container.ts (1)
31-36: LGTM! Correct DI configuration for the updated GitHubService.The options object correctly maps environment configuration values to the
GitHubServiceOptionstype expected by the newGitHubServiceconstructor. The property names align properly with the service's requirements.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/deploy-web/src/components/new-deployment/NewDeploymentPage/createServerSideProps.ts (1)
25-35: Fix undefinedtemplateIdin repoUrl-only flow.
WhenrepoUrlis present buttemplateIdisn’t, you fetch a template usingCI_CD_TEMPLATE_IDbut still returntemplateId: undefined. This regresses the repoUrl-only path. Return the resolved ID (or the template’s id) when a template is found, and avoid passingundefined.🔧 Suggested fix
- const template = await services.template.findById(query.templateId || CI_CD_TEMPLATE_ID).catch(error => { + const resolvedTemplateId = query.templateId ?? CI_CD_TEMPLATE_ID; + const template = await services.template.findById(resolvedTemplateId).catch(error => { services.logger.warn({ error }); return null; }); - if (template && query.templateId) { - return { props: { template, templateId: query.templateId } }; + if (template && query.templateId) { + return { props: { template, templateId: resolvedTemplateId } }; } - return { props: { templateId: query.templateId } }; + if (template) { + return { props: { templateId: resolvedTemplateId } }; + } + + return { props: query.templateId ? { templateId: resolvedTemplateId } : {} };
🤖 Fix all issues with AI agents
In `@apps/deploy-web/src/services/remote-deploy/github-http.service.ts`:
- Around line 47-53: In reLoginWithGithub(), the redirect_uri query is being
inlined and not URL-encoded causing the inner “?step=…” to be treated as
top-level OAuth params; instead construct the redirect URI using the URL and
URLSearchParams APIs (e.g., create a new URL from this.#options.redirectUrl,
append step=edit-deployment and type=github via searchParams) then use its
toString() (or encodeURIComponent if composing manually) when building the OAuth
URL with client_id; update reLoginWithGithub to throw if githubClientId missing,
build the properly encoded redirect_uri via URL/URLSearchParams, and set
window.location.href to the OAuth authorize URL with that encoded redirect_uri.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/deploy-web/src/components/new-deployment/NewDeploymentPage/createServerSideProps.tsapps/deploy-web/src/services/app-di-container/browser-di-container.tsapps/deploy-web/src/services/remote-deploy/github-http.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.
Files:
apps/deploy-web/src/components/new-deployment/NewDeploymentPage/createServerSideProps.tsapps/deploy-web/src/services/app-di-container/browser-di-container.tsapps/deploy-web/src/services/remote-deploy/github-http.service.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
🧬 Code graph analysis (2)
apps/deploy-web/src/components/new-deployment/NewDeploymentPage/createServerSideProps.ts (1)
apps/deploy-web/src/config/remote-deploy.config.ts (1)
CI_CD_TEMPLATE_ID(3-3)
apps/deploy-web/src/services/remote-deploy/github-http.service.ts (3)
apps/deploy-web/src/types/remotedeploy.ts (1)
GithubRepository(2-2)apps/deploy-web/src/types/remoteProfile.ts (1)
GitHubProfile(2-2)apps/deploy-web/src/types/remoteCommits.ts (1)
GitCommit(2-2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-build
🔇 Additional comments (6)
apps/deploy-web/src/components/new-deployment/NewDeploymentPage/createServerSideProps.ts (1)
21-23: Guard for missing inputs looks good.
Prevents unnecessary lookups when neithertemplateIdnorrepoUrlis present.apps/deploy-web/src/services/app-di-container/browser-di-container.ts (1)
31-36: LGTM: GitHubService options wiring looks correct.Nice alignment with the new constructor signature and public config wiring.
apps/deploy-web/src/services/remote-deploy/github-http.service.ts (4)
18-41: Solid encapsulation with private clients + options.Constructor/fields are clean and match the intended DI usage.
43-45: LGTM: direct app installation redirect.Simple and aligned with the new installation URL flow.
64-124: Nice: installations-based pagination with dedupe.The installations flow with
Promise.allSettled+ Map dedup reads clean and resilient.
126-164: LGTM: API calls consistently use the private clients.Consistent use of
#githubApiServiceand#internalApiServicekeeps the surface tidy.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
ygrishajev
force-pushed
the
fix/deployment
branch
from
7ee1527 to
97f2bc5
Compare
ygrishajev
force-pushed
the
fix/deployment
branch
from
97f2bc5 to
8ef5689
Compare
3 participants
refs #2509, #2511
Summary by CodeRabbit
New Features
Bug Fixes
Behavior Change
UI / Stability
✏️ Tip: You can customize this high-level summary in your review settings.