fix(sites): reconnect repo flow and branch selection#2899
Open
HarshMN2345 wants to merge 6 commits intomainfrom
Open
fix(sites): reconnect repo flow and branch selection#2899HarshMN2345 wants to merge 6 commits intomainfrom
HarshMN2345 wants to merge 6 commits intomainfrom
Conversation
Console (appwrite/console)Project ID: Tip Function scopes give you fine-grained control over API permissions |
Member
Author
|
@greptile |
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughUpdates repository connection flows to be asynchronous and surface success/error notifications in connectRepoModal.svelte and repositories.svelte. Deployment and repository-update flows now fetch and sort repository branches to choose a sensible default branch (preferring the current providerBranch, then main/master, then first branch) instead of hardcoding "main", and they propagate optional fields providerSilentMode and providerRootDirectory in site update payloads. Error handling is changed to rethrow on failures in connect flows. package.json updates overrides.minimatch from 10.2.1 to 10.2.3. No exported/public signatures changed. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/settings/updateRepository.svelte (1)
118-160:⚠️ Potential issue | 🟠 MajorDo not swallow
connectupdate failures.At Line 158-Line 160, errors are swallowed and the promise resolves. Callers that await
connectcan treat failures as success and show incorrect success feedback.Suggested fix
- } catch { - return; + } catch (error) { + throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(console)/project-[region]-[project]/sites/site-[site]/settings/updateRepository.svelte around lines 118 - 160, The outer try/catch in the connect function is swallowing all errors (including failures from sdk.forProject(...).sites.update), causing callers awaiting connect to see success; remove the empty outer catch or rethrow the caught error so failures propagate. Keep the inner try/catch around the branch lookup (which can safely fallback), but ensure any exception from sites.update (or other parts of connect) is either thrown upward or logged and rethrown from connect so callers can handle/display failure; reference the connect function and the sdk.forProject(...).sites.update call when making this change.src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/+page.svelte (1)
43-85:⚠️ Potential issue | 🟠 MajorPropagate
connectfailures to caller instead of returning silently.At Line 82-Line 84, failures are swallowed, so upstream UI flows awaiting
connectcannot reliably distinguish success from failure.Suggested fix
- } catch { - return; + } catch (error) { + throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(console)/project-[region]-[project]/sites/site-[site]/deployments/+page.svelte around lines 43 - 85, The connect function currently swallows all errors in its outer try/catch (the catch at the end of connect), preventing callers from knowing if the operation failed; change that outer catch to rethrow the caught error (or return a rejected Promise) so callers can handle failures, while preserving the inner catch that ignores branch lookup failures; locate the async function connect and replace the final catch block so it either throws the caught error (e.g., throw error) or rethrows, optionally logging before rethrowing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/components/git/repositories.svelte`:
- Around line 279-303: The current click handler races connect(repo) against a
timeout but leaves the original connect call running and clears
connectingRepositoryId on timeout, causing conflicting UX; fix by making the
attempt cancellable or by using a local attempt token so late resolutions are
ignored: create an AbortController and pass its signal to connect(repo) if
connect supports abort (use connectTimeoutMs to call controller.abort() on
timeout), otherwise generate a unique attemptId local variable and store it
(e.g., currentAttemptId) before awaiting the race, then only call
addNotification and clear connectingRepositoryId when the resolved/rejected
promise belongs to the same attemptId; ensure any timeout path does not
prematurely clear connectingRepositoryId unless the in-flight call is actually
aborted or the token matches.
In
`@src/routes/`(console)/project-[region]-[project]/sites/site-[site]/deployments/+page.svelte:
- Around line 77-79: The payload currently uses "|| undefined" for
providerSilentMode, providerRootDirectory, and specification which drops
explicit falsy values (false or empty string); change those fallbacks to use the
nullish coalescing operator (?? undefined) or remove the fallback so the raw
values from data.site?.providerSilentMode, data.site?.providerRootDirectory, and
data.site?.specification are preserved when they are valid falsy values.
In
`@src/routes/`(console)/project-[region]-[project]/sites/site-[site]/settings/updateRepository.svelte:
- Around line 151-153: The code that builds the update payload is using
`site?.providerSilentMode || undefined` and `site?.providerRootDirectory ||
undefined`, which turns valid falsy values (e.g. false or empty string) into
undefined; change these to use nullish coalescing so only null/undefined are
collapsed: replace the `|| undefined` usages for `providerSilentMode` and
`providerRootDirectory` with `?? undefined` (leave `providerBranch: nextBranch`
unchanged) so falsy but valid config values are preserved when constructing the
update payload.
---
Outside diff comments:
In
`@src/routes/`(console)/project-[region]-[project]/sites/site-[site]/deployments/+page.svelte:
- Around line 43-85: The connect function currently swallows all errors in its
outer try/catch (the catch at the end of connect), preventing callers from
knowing if the operation failed; change that outer catch to rethrow the caught
error (or return a rejected Promise) so callers can handle failures, while
preserving the inner catch that ignores branch lookup failures; locate the async
function connect and replace the final catch block so it either throws the
caught error (e.g., throw error) or rethrows, optionally logging before
rethrowing.
In
`@src/routes/`(console)/project-[region]-[project]/sites/site-[site]/settings/updateRepository.svelte:
- Around line 118-160: The outer try/catch in the connect function is swallowing
all errors (including failures from sdk.forProject(...).sites.update), causing
callers awaiting connect to see success; remove the empty outer catch or rethrow
the caught error so failures propagate. Keep the inner try/catch around the
branch lookup (which can safely fallback), but ensure any exception from
sites.update (or other parts of connect) is either thrown upward or logged and
rethrown from connect so callers can handle/display failure; reference the
connect function and the sdk.forProject(...).sites.update call when making this
change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/components/git/connectRepoModal.sveltesrc/lib/components/git/repositories.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/+page.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/settings/updateRepository.svelte
src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/+page.svelte
Outdated
Show resolved
Hide resolved
...utes/(console)/project-[region]-[project]/sites/site-[site]/settings/updateRepository.svelte
Outdated
Show resolved
Hide resolved
Contributor
Greptile SummaryThis PR improves the repository reconnection flow for sites with better error handling and intelligent branch selection. Key improvements:
Changes address:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Modal as ConnectRepoModal
participant Page as Deployments/Settings
participant SDK as Appwrite SDK
participant API as VCS API
User->>Modal: Click "Connect Repository"
Modal->>Page: connect(installationId, repositoryId)
Page->>SDK: listRepositoryBranches()
SDK->>API: GET /vcs/branches
alt Branch fetch succeeds
API-->>SDK: Return branch list
SDK-->>Page: branches[]
Page->>Page: sortBranches(branches)
Page->>Page: Select branch:<br/>1. Current branch if exists<br/>2. main/master if exists<br/>3. First sorted branch<br/>4. Fallback to 'main'
else Branch fetch fails
API-->>SDK: Error
SDK-->>Page: Error
Page->>Page: Use fallback branch<br/>(current or 'main')
end
Page->>SDK: sites.update()
Note over Page,SDK: Includes: repository, branch,<br/>silentMode, rootDirectory
SDK->>API: PATCH /sites/{siteId}
API-->>SDK: Updated site
SDK-->>Page: Success
Page->>Page: invalidate(Dependencies.SITE)
Page-->>Modal: Success
Modal->>Modal: Close modal
Modal->>User: Show success notification
Last reviewed commit: 90912c3 |
Comment on lines
54
to
57
| nextBranch = | ||
| sorted.find((branch) => branch.name === data.site?.providerBranch)?.name ?? | ||
| sorted[0]?.name ?? | ||
| nextBranch; |
Contributor
There was a problem hiding this comment.
When reconnecting to a new repo, consider adding explicit check for main/master branches before sorted[0] fallback, since sortBranches may not always prioritize them correctly.
Suggested change
| nextBranch = | |
| sorted.find((branch) => branch.name === data.site?.providerBranch)?.name ?? | |
| sorted[0]?.name ?? | |
| nextBranch; | |
| nextBranch = | |
| sorted.find((branch) => branch.name === data.site?.providerBranch)?.name ?? | |
| sorted.find((branch) => branch.name === 'main' || branch.name === 'master')?.name ?? | |
| sorted[0]?.name ?? | |
| nextBranch; |
Comment on lines
129
to
132
| nextBranch = | ||
| sorted.find((branch) => branch.name === site?.providerBranch)?.name ?? | ||
| sorted[0]?.name ?? | ||
| nextBranch; |
Contributor
There was a problem hiding this comment.
Same as deployments page - consider explicit main/master check before sorted[0] fallback.
Suggested change
| nextBranch = | |
| sorted.find((branch) => branch.name === site?.providerBranch)?.name ?? | |
| sorted[0]?.name ?? | |
| nextBranch; | |
| nextBranch = | |
| sorted.find((branch) => branch.name === site?.providerBranch)?.name ?? | |
| sorted.find((branch) => branch.name === 'main' || branch.name === 'master')?.name ?? | |
| sorted[0]?.name ?? | |
| nextBranch; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/settings/updateRepository.svelte (1)
120-137: Extract branch-resolution logic into a shared helper.The same
listRepositoryBranches + sort + fallbackflow is duplicated here and in deployments. Centralizing it will reduce drift and future bug fixes in one place.♻️ Refactor sketch
- let nextBranch = site?.providerBranch ?? 'main'; - try { - const branchList = await sdk - .forProject(page.params.region, page.params.project) - .vcs.listRepositoryBranches({ - installationId: selectedInstallationId, - providerRepositoryId: selectedRepository - }); - const sorted = sortBranches(branchList.branches); - nextBranch = - sorted.find((branch) => branch.name === site?.providerBranch)?.name ?? - sorted.find((branch) => branch.name === 'main' || branch.name === 'master') - ?.name ?? - sorted[0]?.name ?? - nextBranch; - } catch { - // Ignore branch lookup failures; fallback to default. - } + const nextBranch = await resolveNextBranch({ + sdk, + region: page.params.region, + project: page.params.project, + installationId: selectedInstallationId, + providerRepositoryId: selectedRepository, + preferredBranch: site?.providerBranch, + fallbackBranch: 'main' + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(console)/project-[region]-[project]/sites/site-[site]/settings/updateRepository.svelte around lines 120 - 137, Extract the duplicated branch-resolution flow into a shared async helper (e.g., resolveRepositoryBranch) that accepts the sdk/project identifiers (region, project), installationId, providerRepositoryId, and preferredBranch (site?.providerBranch) and returns the resolved branch name; inside the helper call sdk.forProject(...).vcs.listRepositoryBranches, reuse sortBranches, apply the same fallback order (preferredBranch, 'main'|'master', first branch, then provided default) and catch errors to return the supplied default, then replace the inline block (the code using nextBranch, listRepositoryBranches, sortBranches) with a single call to resolveRepositoryBranch and ensure both this file and the deployments usage call that same helper so the behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/routes/`(console)/project-[region]-[project]/sites/site-[site]/settings/updateRepository.svelte:
- Around line 120-137: Extract the duplicated branch-resolution flow into a
shared async helper (e.g., resolveRepositoryBranch) that accepts the sdk/project
identifiers (region, project), installationId, providerRepositoryId, and
preferredBranch (site?.providerBranch) and returns the resolved branch name;
inside the helper call sdk.forProject(...).vcs.listRepositoryBranches, reuse
sortBranches, apply the same fallback order (preferredBranch, 'main'|'master',
first branch, then provided default) and catch errors to return the supplied
default, then replace the inline block (the code using nextBranch,
listRepositoryBranches, sortBranches) with a single call to
resolveRepositoryBranch and ensure both this file and the deployments usage call
that same helper so the behavior remains identical.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/components/git/repositories.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/+page.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/settings/updateRepository.svelte
Contributor
Additional Comments (2)
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/settings/updateRepository.svelte (1)
120-134: Extract branch-resolution logic into a shared helper.This block is duplicated in both sites connect flows (
settings/updateRepository.svelteanddeployments/+page.svelte), which increases drift risk over time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(console)/project-[region]-[project]/sites/site-[site]/settings/updateRepository.svelte around lines 120 - 134, Extract the duplicated branch-resolution logic into a shared helper (e.g., resolveDefaultBranch) that takes the raw branch list (branchList.branches) and an optional preferred branch (site?.providerBranch) and returns the chosen branch name; inside the helper call sortBranches, then apply the current selection order (preferred branch → 'main' or 'master' → first sorted branch → fallback default), and replace the inline logic in both updateRepository.svelte and deployments/+page.svelte so they call the helper after awaiting sdk.forProject(...).vcs.listRepositoryBranches({ installationId: selectedInstallationId, providerRepositoryId: selectedRepository }); ensure the helper signature and callers accept and return the same types and preserve the existing fallback variable (nextBranch) behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/routes/`(console)/project-[region]-[project]/sites/site-[site]/settings/updateRepository.svelte:
- Around line 135-137: The try/catch that performs the branch lookup in
updateRepository.svelte should not silently swallow errors; change the catch to
capture the error (e.g., catch (err)) and log or surface its details before
falling back to the default branch so permission/API failures are
visible—specifically update the catch around the branch lookup logic (the block
that falls back to defaultBranch/selectedBranch) to call console.error or the
app logger with a descriptive message and the caught error (and only suppress
the exception after logging).
---
Nitpick comments:
In
`@src/routes/`(console)/project-[region]-[project]/sites/site-[site]/settings/updateRepository.svelte:
- Around line 120-134: Extract the duplicated branch-resolution logic into a
shared helper (e.g., resolveDefaultBranch) that takes the raw branch list
(branchList.branches) and an optional preferred branch (site?.providerBranch)
and returns the chosen branch name; inside the helper call sortBranches, then
apply the current selection order (preferred branch → 'main' or 'master' → first
sorted branch → fallback default), and replace the inline logic in both
updateRepository.svelte and deployments/+page.svelte so they call the helper
after awaiting sdk.forProject(...).vcs.listRepositoryBranches({ installationId:
selectedInstallationId, providerRepositoryId: selectedRepository }); ensure the
helper signature and callers accept and return the same types and preserve the
existing fallback variable (nextBranch) behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/+page.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/settings/updateRepository.svelte
Comment on lines
135
to
137
| } catch { | ||
| // Ignore branch lookup failures; fallback to default. | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t fully suppress branch lookup failures before fallback.
Fallback behavior is fine, but swallowing all errors here makes repo/API permission failures hard to diagnose and can mask why a non-default branch was chosen.
Suggested change
- } catch {
- // Ignore branch lookup failures; fallback to default.
+ } catch (error) {
+ console.warn(
+ 'Failed to fetch repository branches; falling back to default branch.',
+ error
+ );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/routes/`(console)/project-[region]-[project]/sites/site-[site]/settings/updateRepository.svelte
around lines 135 - 137, The try/catch that performs the branch lookup in
updateRepository.svelte should not silently swallow errors; change the catch to
capture the error (e.g., catch (err)) and log or surface its details before
falling back to the default branch so permission/API failures are
visible—specifically update the catch around the branch lookup logic (the block
that falls back to defaultBranch/selectedBranch) to call console.error or the
app logger with a descriptive message and the caught error (and only suppress
the exception after logging).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit
Bug Fixes
New Features
Chores