feat: auto-fill detected env variables for sites/functions#2851
feat: auto-fill detected env variables for sites/functions#2851HarshMN2345 wants to merge 6 commits intomainfrom
Conversation
Console (appwrite/console)Project ID: Tip Each function runs in its own isolated container with custom environment variables |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a reusable EnvironmentVariables Svelte component and related modal/editor components, centralizing environment-variable UI and replacing prior inline modal logic in site configuration. Introduces helpers Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
![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
`@src/routes/`(console)/project-[region]-[project]/sites/create-site/repositories/repository-[repository]/+page.svelte:
- Around line 52-89: Extract the duplicated DetectedVariable type and the
functions normalizeDetectedVariables and mergeVariables into a single shared
helper module (e.g., helpers/variables.ts), export them, and import them from
both pages to eliminate duplication; keep the exact function signatures and
behavior (using Partial<Models.Variable> and the same key/name/value/secret
handling), ensure to import type { Models } from '@appwrite.io/console' in the
helper, remove the inline definitions from each +page.svelte, and replace them
with imports of DetectedVariable, normalizeDetectedVariables, and
mergeVariables.
🧹 Nitpick comments (1)
src/lib/components/variables/environmentVariables.svelte (1)
186-207: Merge adjacent identical{#if!variable?.secret}blocks.Two consecutive blocks check the same condition. Combining them reduces template noise and makes it clearer these actions are grouped.
♻️ Proposed merge
- {`#if` !variable?.secret} - <ActionMenu.Item.Button - leadingIcon={IconPencil} - on:click={(e) => { - toggle(e); - currentVariable = variable; - showUpdate = true; - }}> - Update - </ActionMenu.Item.Button> - {/if} - {`#if` !variable?.secret} - <ActionMenu.Item.Button - leadingIcon={IconEyeOff} - on:click={(e) => { - toggle(e); - currentVariable = variable; - showSecretModal = true; - }}> - Secret - </ActionMenu.Item.Button> - {/if} + {`#if` !variable?.secret} + <ActionMenu.Item.Button + leadingIcon={IconPencil} + on:click={(e) => { + toggle(e); + currentVariable = variable; + showUpdate = true; + }}> + Update + </ActionMenu.Item.Button> + <ActionMenu.Item.Button + leadingIcon={IconEyeOff} + on:click={(e) => { + toggle(e); + currentVariable = variable; + showSecretModal = true; + }}> + Secret + </ActionMenu.Item.Button> + {/if}
...oject-[region]-[project]/sites/create-site/repositories/repository-[repository]/+page.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
- Svelte 5 format.
productLabelshould have proper union type,site | function | global.
|
|
||
| export let variables: Partial<Models.Variable>[] = []; | ||
| export let productLabel = 'site'; | ||
| export let docsLink = |
There was a problem hiding this comment.
we should do this links building in the component based on productLabel just once.
| <Button | ||
| secondary | ||
| size="s" | ||
| on:mousedown={() => { |
There was a problem hiding this comment.
does this directive work on components too?? 👀
There was a problem hiding this comment.
on:click() should work 👍
| columns={[ | ||
| { id: 'key', width: { min: 300 } }, | ||
| { id: 'value', width: { min: 280 } }, | ||
| { id: 'actions', width: 40 } | ||
| ]}> |
There was a problem hiding this comment.
used twice, extract to a const.
src/lib/helpers/variables.ts
Outdated
| key?: string; | ||
| name?: string; | ||
| value?: string; | ||
| secret?: boolean; |
There was a problem hiding this comment.
how come all are optional? I'd expect key and the name to be there. if not, I don't think it constitutes as a valid env format.
There was a problem hiding this comment.
in what cases does this happen?
There was a problem hiding this comment.
value can be empty because we often detect from .env.example / .env.local, which are intentionally key‑only placeholders
| bind:buildCommand | ||
| bind:roles | ||
| bind:variables | ||
| isVariablesLoading={detectingRuntime} /> |
There was a problem hiding this comment.
naming sounds confusing to me. one variable oriented, another runtime.
There was a problem hiding this comment.
isLoading should work
| import { InputText } from '$lib/elements/forms'; | ||
| import { Accordion, Fieldset, Layout } from '@appwrite.io/pink-svelte'; | ||
| import type { Models } from '@appwrite.io/console'; | ||
| import EnvironmentVariables from '$lib/components/variables/environmentVariables.svelte'; |
There was a problem hiding this comment.
lets just make an index.ts [barrel file] and export all variables/* from there. better management and readability.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/components/variables/environmentVariables.svelte`:
- Around line 233-234: The Empty-state click handler currently only sets
showCreate = true and misses analytics; update the Empty on:click to mirror the
Create button by firing the same analytics event (Click.VariablesCreateClick)
before toggling showCreate so the first-variable creation path is tracked;
locate the Empty component usage and add the analytics call (using the existing
tracking function used by the Create variable button) then set showCreate =
true.
🧹 Nitpick comments (2)
src/lib/components/variables/environmentVariables.svelte (2)
60-60:currentVariableinitialized asundefinedbut typed asPartial<Models.Variable>.Since the modals that consume
currentVariableare guarded by{#ifshowXxx}and the show flags are only set aftercurrentVariableis assigned (e.g., lines 196–197), this won't cause runtime errors in practice. However, the type is technically inaccurate.💡 Optional: make the type honest
- let currentVariable = $state<Partial<Models.Variable>>(undefined); + let currentVariable = $state<Partial<Models.Variable> | undefined>(undefined);
190-212: Two consecutive{#if!variable?.secret}blocks could be merged.Lines 191 and 202 guard on the same condition. Combining them into a single block reduces template nesting and makes the intent clearer.
Suggested simplification
<ActionMenu.Root> - {`#if` !variable?.secret} - <ActionMenu.Item.Button - leadingIcon={IconPencil} - onclick={(e) => { - toggle(e); - currentVariable = variable; - showUpdate = true; - }}> - Update - </ActionMenu.Item.Button> - {/if} - {`#if` !variable?.secret} - <ActionMenu.Item.Button - leadingIcon={IconEyeOff} - onclick={(e) => { - toggle(e); - currentVariable = variable; - showSecretModal = true; - }}> - Secret - </ActionMenu.Item.Button> - {/if} + {`#if` !variable?.secret} + <ActionMenu.Item.Button + leadingIcon={IconPencil} + onclick={(e) => { + toggle(e); + currentVariable = variable; + showUpdate = true; + }}> + Update + </ActionMenu.Item.Button> + <ActionMenu.Item.Button + leadingIcon={IconEyeOff} + onclick={(e) => { + toggle(e); + currentVariable = variable; + showSecretModal = true; + }}> + Secret + </ActionMenu.Item.Button> + {/if}
| {:else} | ||
| <Empty on:click={() => (showCreate = true)}>Create variables to get started</Empty> |
There was a problem hiding this comment.
Missing analytics tracking when creating a variable from the empty state.
The "Create variable" button (lines 105–109) tracks Click.VariablesCreateClick, but the Empty state click handler does not. This creates an analytics blind spot for the first-variable creation path.
Proposed fix
- <Empty on:click={() => (showCreate = true)}>Create variables to get started</Empty>
+ <Empty on:click={() => {
+ showCreate = true;
+ trackEvent(Click.VariablesCreateClick, {
+ source: createSource
+ });
+ }}>Create variables to get started</Empty>🤖 Prompt for AI Agents
In `@src/lib/components/variables/environmentVariables.svelte` around lines 233 -
234, The Empty-state click handler currently only sets showCreate = true and
misses analytics; update the Empty on:click to mirror the Create button by
firing the same analytics event (Click.VariablesCreateClick) before toggling
showCreate so the first-variable creation path is tracked; locate the Empty
component usage and add the analytics call (using the existing tracking function
used by the Create variable button) then set showCreate = true.
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)
src/routes/(console)/project-[region]-[project]/functions/create-function/repository-[repository]/+page.svelte (1)
126-133:⚠️ Potential issue | 🟡 MinorPre-existing:
variable.keyandvariable.valuemay beundefinedforPartial<Models.Variable>.Since
variablesis typed asPartial<Models.Variable>[], the SDK call at lines 129–130 could receiveundefinedforkeyorvalue. With the new auto-fill flow introducing additional variable sources, consider filtering out incomplete entries before creating them.🛡️ Suggested defensive filter
- const promises = variables.map((variable) => + const promises = variables.filter((v) => v.key && v.value).map((variable) => sdk.forProject(page.params.region, page.params.project).functions.createVariable({
🤖 Fix all issues with AI agents
In
`@src/routes/`(console)/project-[region]-[project]/functions/create-function/repository-[repository]/+page.svelte:
- Around line 86-89: normalizeDetectedVariables currently only defaults for
undefined and will throw if detections?.variables is null; update its signature
or body to accept Models.DetectionVariable[] | null | undefined = [] (or check
for null at start) so it treats null like an empty array before iterating, then
continue using it where called (e.g., the call in this file that assigns
detectedVariables and merges via mergeVariables) to avoid runtime errors when
the API returns variables: null.
🧹 Nitpick comments (1)
src/routes/(console)/project-[region]-[project]/sites/create-site/repositories/repository-[repository]/+page.svelte (1)
127-137: Pre-existing:Promise.allfor variable creation has no partial-failure handling.If one variable creation fails,
Promise.allrejects immediately while other requests may still be in-flight. The site and domain will already have been created, potentially leaving partial state. This is pre-existing behavior and not introduced by this PR, but worth noting for future improvement (e.g.,Promise.allSettledwith error aggregation).
| const detectedVariables = normalizeDetectedVariables(detections?.variables); | ||
| if (detectedVariables.length) { | ||
| variables = mergeVariables(variables, detectedVariables); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for normalizeDetectedVariables function definition
rg -n 'normalizeDetectedVariables' --type=ts --type=js --type=svelte -B2 -A10Repository: appwrite/console
Length of output: 90
🏁 Script executed:
# Search for normalizeDetectedVariables function definition
rg -n 'normalizeDetectedVariables' -B2 -A10Repository: appwrite/console
Length of output: 9202
🏁 Script executed:
# Get the complete implementation of normalizeDetectedVariables
cat -n src/lib/helpers/variables.ts | head -30Repository: appwrite/console
Length of output: 1119
🏁 Script executed:
# Find where detections is declared/assigned in the function
rg -n 'const detections' src/routes/\(console\)/project-\[region\]-\[project\]/functions/create-function/repository-\[repository\]/+page.svelte -B5 -A2Repository: appwrite/console
Length of output: 333
🏁 Script executed:
# Get more context around the detections assignment
rg -n 'const detections = ' src/routes/\(console\)/project-\[region\]-\[project\]/functions/create-function/repository-\[repository\]/+page.svelte -A15Repository: appwrite/console
Length of output: 909
🏁 Script executed:
# Check the sites version for comparison
rg -n 'normalizeDetectedVariables' src/routes/\(console\)/project-\[region\]-\[project\]/sites/create-site/repositories/repository-\[repository\]/+page.svelte -B5 -A5Repository: appwrite/console
Length of output: 1205
Handle null in addition to undefined for detections?.variables.
normalizeDetectedVariables has a default parameter = [] that only catches undefined, not null. If the API response includes variables: null, the function will throw a runtime error when trying to iterate. Update the function signature to detected: Models.DetectionVariable[] | null | undefined = [] or add an explicit null check in the function body to handle both falsy cases gracefully.
🤖 Prompt for AI Agents
In
`@src/routes/`(console)/project-[region]-[project]/functions/create-function/repository-[repository]/+page.svelte
around lines 86 - 89, normalizeDetectedVariables currently only defaults for
undefined and will throw if detections?.variables is null; update its signature
or body to accept Models.DetectionVariable[] | null | undefined = [] (or check
for null at start) so it treats null like an empty array before iterating, then
continue using it where called (e.g., the call in this file that assigns
detectedVariables and merges via mergeVariables) to avoid runtime errors when
the API returns variables: null.
3 participants
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
example:
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
New Features
Chore