Conversation
Console (appwrite/console)Project ID: Tip Custom domains work with both CNAME for subdomains and NS records for apex domains |
WalkthroughThis pull request extends the deploymentSource component with VCS authorization checks. A new authorization flow is introduced in deploymentSource.svelte that fetches repository authorization status on mount using the SDK store and displays a warning tooltip when authorization fails. The siteCard and deploymentCard components are updated to pass resource, region, and project context to DeploymentSource. The page component passes the site object to siteCard via a new prop, enabling the authorization context to flow through the component tree. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Greptile SummaryReplaced Alert.Inline banners with compact warning icons next to GitHub links when VCS integration is not authorized for auto-deployments. The authorization check now happens in the
Note: The PR description mentions "parallelizing deployment and repository data fetching" but the current implementation fetches repository data after page load in Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Page
participant Loader as +page.ts
participant Component as deploymentSource.svelte
participant API as VCS API
User->>Page: Navigate to site/function
Page->>Loader: Load page data
Loader->>API: Fetch deployment data
API-->>Loader: Return deployment
Loader-->>Page: Return page data
Page->>Component: Mount with deployment
Component->>Component: onMount()
Component->>API: getRepository(installationId, providerRepositoryId)
API-->>Component: Return repository.authorized
Component->>Component: Update authorized state
Component->>User: Show warning icon if authorized === false
Last reviewed commit: de1ef99 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/routes/(console)/project-[region]-[project]/functions/function-[function]/+page.svelte
Outdated
Show resolved
Hide resolved
src/routes/(console)/project-[region]-[project]/sites/site-[site]/+page.svelte
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/+page.ts (1)
103-110: Avoid swallowing all fetch errors silently.At Line 108 and Line 129, returning
nullfor every exception hides operational failures. Consider at least logging unexpected errors so production issues remain debuggable.Suggested pattern
async function loadDeployment({ @@ }): Promise<Models.Deployment | null> { try { return await sdk.forProject(region, project).sites.getDeployment({ siteId, deploymentId }); } catch (error) { + console.error('Failed to load site deployment', { siteId, deploymentId, error }); return null; } } @@ }): Promise<Models.ProviderRepository | null> { try { return await sdk.forProject(region, project).vcs.getRepository({ installationId, providerRepositoryId }); } catch (error) { + console.error('Failed to load site repository', { + installationId, + providerRepositoryId, + error + }); return null; } }Also applies to: 124-131
🤖 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]/+page.ts around lines 103 - 110, The catch block that wraps sdk.forProject(...).sites.getDeployment currently swallows all errors and returns null; change it to detect and handle expected "not found" responses (e.g., check error.status === 404 or SDK-specific NotFound error) returning null only in that case, but for all other exceptions log the error with context (include region, project, siteId, deploymentId) using the project's logger (or console.error if no logger available) and rethrow or return a meaningful error; update the catch around sdk.forProject(...).sites.getDeployment (and the similar block calling the same method) accordingly so operational failures are not silently hidden.
🤖 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]/functions/function-[function]/+page.ts:
- Around line 69-70: The load function currently mutates the module-scoped
queries store by calling queries.set(parsedQueries) after computing
parsedQueries with queryParamToMap(query || '[]'); remove that mutation from the
load function and instead return parsedQueries as part of the load data payload
(e.g., return { parsedQueries }). Then initialize the queries store on the
client: subscribe/ set it inside a client-only context such as a +page.svelte
onMount or a script tag with context="module" avoided—use onMount to call
queries.set(data.parsedQueries) or otherwise subscribe to load data; update the
+page.svelte to read the returned parsedQueries and set the queries store there
so SSR does not mutate shared module state.
In
`@src/routes/`(console)/project-[region]-[project]/sites/site-[site]/+page.svelte:
- Around line 39-44: Replace the generic link text "here" in the Link component
with descriptive copy that conveys the destination (e.g., "Manage GitHub App
installation" or "View GitHub installation settings") and update the Link
(component named Link, href constructed with
data.repository.providerInstallationId) to include an accessible label if
additional context is needed (aria-label) so screen readers see the destination
out of context; ensure the text mentions GitHub and installation/settings to
match the URL.
---
Nitpick comments:
In `@src/routes/`(console)/project-[region]-[project]/sites/site-[site]/+page.ts:
- Around line 103-110: The catch block that wraps
sdk.forProject(...).sites.getDeployment currently swallows all errors and
returns null; change it to detect and handle expected "not found" responses
(e.g., check error.status === 404 or SDK-specific NotFound error) returning null
only in that case, but for all other exceptions log the error with context
(include region, project, siteId, deploymentId) using the project's logger (or
console.error if no logger available) and rethrow or return a meaningful error;
update the catch around sdk.forProject(...).sites.getDeployment (and the similar
block calling the same method) accordingly so operational failures are not
silently hidden.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/+page.sveltesrc/routes/(console)/project-[region]-[project]/functions/function-[function]/+page.tssrc/routes/(console)/project-[region]-[project]/sites/site-[site]/+page.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/+page.ts
| const parsedQueries = queryParamToMap(query || '[]'); | ||
| queries.set(parsedQueries); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== filters store definition =="
fd 'store.ts' src/lib/components/filters --exec sed -n '1,160p' {}
echo
echo "== route loaders mutating queries store =="
rg -n -C3 "queries\.set\(" --type ts src/routesRepository: appwrite/console
Length of output: 17142
Move store mutation to client-only context; SSR load functions mutate shared module state.
The queries store is module-scoped (defined once in src/lib/components/filters/store.ts). Calling queries.set() in load functions executes on the server during SSR, where module state persists across concurrent requests. This causes filter state from one request/user to leak into subsequent requests.
Move the mutation outside the load function:
- Return the parsed queries as data from the load function instead of mutating the store
- Subscribe and initialize the store on the client side only
- Or wrap the mutation in a
+page.sveltelifecycle hook that runs client-only
This pattern is repeated across multiple routes (domains, messaging, functions, executions, etc.) and should be addressed consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/routes/`(console)/project-[region]-[project]/functions/function-[function]/+page.ts
around lines 69 - 70, The load function currently mutates the module-scoped
queries store by calling queries.set(parsedQueries) after computing
parsedQueries with queryParamToMap(query || '[]'); remove that mutation from the
load function and instead return parsedQueries as part of the load data payload
(e.g., return { parsedQueries }). Then initialize the queries store on the
client: subscribe/ set it inside a client-only context such as a +page.svelte
onMount or a script tag with context="module" avoided—use onMount to call
queries.set(data.parsedQueries) or otherwise subscribe to load data; update the
+page.svelte to read the returned parsedQueries and set the queries store there
so SSR does not mutate shared module state.
| <Link | ||
| variant="muted" | ||
| external | ||
| href={`https://github.com/settings/installations/${data.repository.providerInstallationId}`}> | ||
| here | ||
| </Link>. |
There was a problem hiding this comment.
Use descriptive link text instead of “here”.
At Line 43, replace generic link text with destination-specific text so users (and assistive tech) can understand it out of context.
Proposed tweak
- here
+ GitHub installation settings🤖 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]/+page.svelte
around lines 39 - 44, Replace the generic link text "here" in the Link component
with descriptive copy that conveys the destination (e.g., "Manage GitHub App
installation" or "View GitHub installation settings") and update the Link
(component named Link, href constructed with
data.repository.providerInstallationId) to include an accessible label if
additional context is needed (aria-label) so screen readers see the destination
out of context; ensure the text mentions GitHub and installation/settings to
match the URL.
| project?: string; | ||
| } = $props(); | ||
|
|
||
| let authorized = $state(null); |
There was a problem hiding this comment.
warning icon could flash in after page loads since authorized starts as null and updates asynchronously
| let authorized = $state(null); | |
| let authorized = $state<boolean | null>(null); |
| async function loadAuthorized() { | ||
| if (!resource?.installationId || !resource?.providerRepositoryId || !region || !project) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const repository = await sdk.forProject(region, project).vcs.getRepository({ | ||
| installationId: resource.installationId, | ||
| providerRepositoryId: resource.providerRepositoryId | ||
| }); | ||
| authorized = repository.authorized; | ||
| } catch (err) { | ||
| console.warn(err); | ||
| } | ||
| } |
There was a problem hiding this comment.
fetching repository data in onMount happens after page load, not in parallel with initial data loading - consider moving this to the page loaders (+page.ts) using Promise.all for true parallelization as mentioned in the PR description
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/components/git/deploymentSource.svelte (1)
29-41: Optional: Add explicit type toauthorizedfor consistency.TypeScript strict null checks are currently disabled in this project, so the current declaration works without issues. However, for consistency with similar state declarations elsewhere (e.g.,
multiSelectTable.svelte), consider explicitly typing the state:♻️ Proposed fix
- let authorized = $state(null); + let authorized = $state<boolean | null>(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/components/git/deploymentSource.svelte` around lines 29 - 41, The local state variable authorized is declared without an explicit type; update the declaration of authorized to include an explicit type (matching other components like multiSelectTable.svelte) such as boolean | null (or the specific Writable/State type you use) so its intended shape is clear and consistent, leaving the existing initializer ($state(null)) and keeping loadAuthorized and its assignment (authorized = repository.authorized) unchanged.
🤖 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/lib/components/git/deploymentSource.svelte`:
- Around line 29-41: The local state variable authorized is declared without an
explicit type; update the declaration of authorized to include an explicit type
(matching other components like multiSelectTable.svelte) such as boolean | null
(or the specific Writable/State type you use) so its intended shape is clear and
consistent, leaving the existing initializer ($state(null)) and keeping
loadAuthorized and its assignment (authorized = repository.authorized)
unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/components/git/deploymentSource.sveltesrc/routes/(console)/project-[region]-[project]/functions/function-[function]/(components)/deploymentCard.sveltesrc/routes/(console)/project-[region]-[project]/sites/(components)/siteCard.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/+page.svelte
1 participant
Towards SER-1129
Summary by CodeRabbit
Release Notes
New Features
Improvements