feat: add site and function migration support#2882
Open
premtsd-code wants to merge 7 commits intomainfrom
Open
feat: add site and function migration support#2882premtsd-code wants to merge 7 commits intomainfrom
premtsd-code wants to merge 7 commits intomainfrom
Conversation
Console (appwrite/console)Project ID: Sites (1)
Tip Sites auto-generate unique domains with the pattern https://randomstring.appwrite.network |
Contributor
WalkthroughThis PR replaces the single Resources enum import with provider-specific migration resource enums (AppwriteMigrationResource, FirebaseMigrationResource, NHostMigrationResource, SupabaseMigrationResource), updates package.json to resolve Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Key observations
🚥 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 |
Contributor
Greptile SummaryThis PR adds support for migrating sites and functions (including environment variables, deployments, and site variables) to the migration wizard. It replaces the generic Major changes:
Critical issues found:
Note: The GitHub branch dependency in Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User selects migration options] --> B[migrationFormToResources]
B --> C{functions.root checked?}
C -->|Yes| D[Add Function resource]
D --> E[Add Environmentvariable]
E --> F[Add Deployment<br/>BUG: Always added regardless of inactive checkbox]
C -->|No| G{sites.root checked?}
G -->|Yes| H[Add Site resource]
H --> I[Add Sitevariable]
I --> J[Add Sitedeployment<br/>BUG: Always added regardless of inactive checkbox]
G -->|No| K{databases.root checked?}
K -->|Yes| L[Add Database/Table/Column/Index]
K -->|No| M{storage.root checked?}
M -->|Yes| N[Add Bucket/File]
M -->|No| O[End]
F --> P[Filter by providerResources]
J --> P
L --> P
N --> P
P --> Q[Return resource array]
style F fill:#ff6b6b
style J fill:#ff6b6b
Last reviewed commit: 0e5a0b8 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
76
to
118
| export const migrationFormToResources = ( | ||
| formData: MigrationFormData, | ||
| provider: Provider | ||
| ): Resources[] => { | ||
| const resources: Resources[] = []; | ||
| const addResource = (resource: Resources) => { | ||
| ): MigrationResource[] => { | ||
| const resources: MigrationResource[] = []; | ||
| const addResource = (resource: MigrationResource) => { | ||
| if (providerResources[provider].includes(resource)) { | ||
| resources.push(resource); | ||
| } | ||
| }; | ||
|
|
||
| if (formData.users.root) { | ||
| addResource(Resources.User); | ||
| addResource(AppwriteMigrationResource.User); | ||
| } | ||
| if (formData.databases.root) { | ||
| addResource(Resources.Database); | ||
| addResource(Resources.Table); | ||
| addResource(Resources.Column); | ||
| addResource(Resources.Index); | ||
| addResource(AppwriteMigrationResource.Database); | ||
| addResource(AppwriteMigrationResource.Table); | ||
| addResource(AppwriteMigrationResource.Column); | ||
| addResource(AppwriteMigrationResource.Index); | ||
| } | ||
| if (formData.databases.rows) { | ||
| addResource(Resources.Row); | ||
| addResource(AppwriteMigrationResource.Row); | ||
| } | ||
| if (formData.storage.root) { | ||
| addResource(Resources.Bucket); | ||
| addResource(Resources.File); | ||
| addResource(AppwriteMigrationResource.Bucket); | ||
| addResource(AppwriteMigrationResource.File); | ||
| } | ||
| if (formData.functions.root) { | ||
| addResource(AppwriteMigrationResource.Function); | ||
| addResource(AppwriteMigrationResource.EnvironmentVariable); | ||
| } | ||
| if (formData.functions.inactive) { | ||
| addResource(AppwriteMigrationResource.Deployment); | ||
| } | ||
| if (formData.sites.root) { | ||
| addResource(AppwriteMigrationResource.Site); | ||
| addResource(AppwriteMigrationResource.SiteVariable); | ||
| } | ||
| if (formData.sites.inactive) { | ||
| addResource(AppwriteMigrationResource.SiteDeployment); | ||
| } | ||
|
|
||
| return resources; |
Contributor
There was a problem hiding this comment.
hardcodes AppwriteMigrationResource enum values for all providers
Suggested change
| export const migrationFormToResources = ( | |
| formData: MigrationFormData, | |
| provider: Provider | |
| ): Resources[] => { | |
| const resources: Resources[] = []; | |
| const addResource = (resource: Resources) => { | |
| ): MigrationResource[] => { | |
| const resources: MigrationResource[] = []; | |
| const addResource = (resource: MigrationResource) => { | |
| if (providerResources[provider].includes(resource)) { | |
| resources.push(resource); | |
| } | |
| }; | |
| if (formData.users.root) { | |
| addResource(Resources.User); | |
| addResource(AppwriteMigrationResource.User); | |
| } | |
| if (formData.databases.root) { | |
| addResource(Resources.Database); | |
| addResource(Resources.Table); | |
| addResource(Resources.Column); | |
| addResource(Resources.Index); | |
| addResource(AppwriteMigrationResource.Database); | |
| addResource(AppwriteMigrationResource.Table); | |
| addResource(AppwriteMigrationResource.Column); | |
| addResource(AppwriteMigrationResource.Index); | |
| } | |
| if (formData.databases.rows) { | |
| addResource(Resources.Row); | |
| addResource(AppwriteMigrationResource.Row); | |
| } | |
| if (formData.storage.root) { | |
| addResource(Resources.Bucket); | |
| addResource(Resources.File); | |
| addResource(AppwriteMigrationResource.Bucket); | |
| addResource(AppwriteMigrationResource.File); | |
| } | |
| if (formData.functions.root) { | |
| addResource(AppwriteMigrationResource.Function); | |
| addResource(AppwriteMigrationResource.EnvironmentVariable); | |
| } | |
| if (formData.functions.inactive) { | |
| addResource(AppwriteMigrationResource.Deployment); | |
| } | |
| if (formData.sites.root) { | |
| addResource(AppwriteMigrationResource.Site); | |
| addResource(AppwriteMigrationResource.SiteVariable); | |
| } | |
| if (formData.sites.inactive) { | |
| addResource(AppwriteMigrationResource.SiteDeployment); | |
| } | |
| return resources; | |
| export const migrationFormToResources = ( | |
| formData: MigrationFormData, | |
| provider: Provider | |
| ): MigrationResource[] => { | |
| const resources: MigrationResource[] = []; | |
| const providerEnum = { | |
| appwrite: AppwriteMigrationResource, | |
| firebase: FirebaseMigrationResource, | |
| nhost: NHostMigrationResource, | |
| supabase: SupabaseMigrationResource | |
| }[provider]; | |
| const addResource = (resource: MigrationResource) => { | |
| if (providerResources[provider].includes(resource)) { | |
| resources.push(resource); | |
| } | |
| }; | |
| if (formData.users.root) { | |
| addResource(providerEnum.User); | |
| } | |
| if (formData.databases.root) { | |
| addResource(providerEnum.Database); | |
| addResource(providerEnum.Table); | |
| addResource(providerEnum.Column); | |
| addResource(providerEnum.Index); | |
| } | |
| if (formData.databases.rows) { | |
| addResource(providerEnum.Row); | |
| } | |
| if (formData.storage.root) { | |
| addResource(providerEnum.Bucket); | |
| addResource(providerEnum.File); | |
| } | |
| if (formData.functions.root) { | |
| addResource(providerEnum.Function); | |
| addResource(providerEnum.EnvironmentVariable); | |
| } | |
| if (formData.functions.inactive) { | |
| addResource(providerEnum.Deployment); | |
| } | |
| if (formData.sites.root) { | |
| addResource(providerEnum.Site); | |
| addResource(providerEnum.SiteVariable); | |
| } | |
| if (formData.sites.inactive) { | |
| addResource(providerEnum.SiteDeployment); | |
| } | |
| return resources; | |
| }; |
Comment on lines
142
to
180
| export const resourcesToMigrationForm = (resources: MigrationResource[]): MigrationFormData => { | ||
| const formData = { ...initialFormData }; | ||
| if (resources.includes(Resources.User)) { | ||
| if (resources.includes(AppwriteMigrationResource.User)) { | ||
| formData.users.root = true; | ||
| } | ||
| if (resources.includes(Resources.Database)) { | ||
| if (resources.includes(AppwriteMigrationResource.Database)) { | ||
| formData.databases.root = true; | ||
| } | ||
| if (includesAll(resources, [Resources.Table, Resources.Column, Resources.Row] as Resources[])) { | ||
| if ( | ||
| includesAll(resources, [ | ||
| AppwriteMigrationResource.Table, | ||
| AppwriteMigrationResource.Column, | ||
| AppwriteMigrationResource.Row | ||
| ] as MigrationResource[]) | ||
| ) { | ||
| formData.databases.rows = true; | ||
| } | ||
| if (includesAll(resources, [Resources.Bucket, Resources.File] as Resources[])) { | ||
| if ( | ||
| includesAll(resources, [ | ||
| AppwriteMigrationResource.Bucket, | ||
| AppwriteMigrationResource.File | ||
| ] as MigrationResource[]) | ||
| ) { | ||
| formData.storage.root = true; | ||
| } | ||
| if (resources.includes(AppwriteMigrationResource.Function)) { | ||
| formData.functions.root = true; | ||
| } | ||
| if (resources.includes(AppwriteMigrationResource.Deployment)) { | ||
| formData.functions.inactive = true; | ||
| } | ||
| if (resources.includes(AppwriteMigrationResource.Site)) { | ||
| formData.sites.root = true; | ||
| } | ||
| if (resources.includes(AppwriteMigrationResource.SiteDeployment)) { | ||
| formData.sites.inactive = true; | ||
| } | ||
|
|
||
| return formData; |
Contributor
There was a problem hiding this comment.
same issue - only checks AppwriteMigrationResource enum values when the input could be from any provider
The function should determine which provider enum to use based on the actual enum values in the input array, or accept a provider parameter.
| "dependencies": { | ||
| "@ai-sdk/svelte": "^1.1.24", | ||
| "@appwrite.io/console": "https://pkg.vc/-/@appwrite/@appwrite.io/console@de65a99", | ||
| "@appwrite.io/console": "github:appwrite/sdk-for-console#migration-resource-enum-fix", |
Contributor
There was a problem hiding this comment.
consider replacing with versioned release before merging to main
This GitHub branch reference is fine for development/testing, but should be replaced with a versioned release (via pkg.vc or npm) for production stability.
Contributor
Additional Comments (1)
When Need to either:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/stores/migration.ts`:
- Around line 167-178: The migration->form conversion currently maps
Function/Deployment/Site/SiteDeployment to formData.functions.root/inactive and
formData.sites.root/inactive but misses mapping EnvironmentVariable and
SiteVariable so the UI env toggles remain off; add checks for
AppwriteMigrationResource.EnvironmentVariable and
AppwriteMigrationResource.SiteVariable and set the corresponding formData
toggles (e.g. set formData.functions.env = true when EnvironmentVariable is
present and set formData.sites.env = true when SiteVariable is present)
alongside the existing mappings in the same block that references
AppwriteMigrationResource.Function/Deployment/Site/SiteDeployment.
- Around line 79-116: The code always adds environment variable resources when
functions.root or sites.root is true, ignoring the specific env toggles; update
the resource-building logic so MigrationResource.EnvironmentVariable is only
added when both formData.functions.root and formData.functions.env are true
(refer to formData.functions and MigrationResource.EnvironmentVariable), and
likewise only add MigrationResource.SiteVariable when both formData.sites.root
and formData.sites.env are true (refer to formData.sites and
MigrationResource.SiteVariable); keep the existing
providerResources[provider].includes(...) guard and use the same
addResource(...) helper.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.jsonsrc/lib/stores/migration.tssrc/routes/(console)/(migration-wizard)/resource-form.sveltesrc/routes/(console)/project-[region]-[project]/settings/migrations/(import)/importReport.svelte
Comment on lines
79
to
116
| ): MigrationResource[] => { | ||
| const resources: MigrationResource[] = []; | ||
| const addResource = (resource: MigrationResource) => { | ||
| if (providerResources[provider].includes(resource)) { | ||
| resources.push(resource); | ||
| } | ||
| }; | ||
|
|
||
| if (formData.users.root) { | ||
| addResource(Resources.User); | ||
| addResource(AppwriteMigrationResource.User); | ||
| } | ||
| if (formData.databases.root) { | ||
| addResource(Resources.Database); | ||
| addResource(Resources.Table); | ||
| addResource(Resources.Column); | ||
| addResource(Resources.Index); | ||
| addResource(AppwriteMigrationResource.Database); | ||
| addResource(AppwriteMigrationResource.Table); | ||
| addResource(AppwriteMigrationResource.Column); | ||
| addResource(AppwriteMigrationResource.Index); | ||
| } | ||
| if (formData.databases.rows) { | ||
| addResource(Resources.Row); | ||
| addResource(AppwriteMigrationResource.Row); | ||
| } | ||
| if (formData.storage.root) { | ||
| addResource(Resources.Bucket); | ||
| addResource(Resources.File); | ||
| addResource(AppwriteMigrationResource.Bucket); | ||
| addResource(AppwriteMigrationResource.File); | ||
| } | ||
| if (formData.functions.root) { | ||
| addResource(AppwriteMigrationResource.Function); | ||
| addResource(AppwriteMigrationResource.EnvironmentVariable); | ||
| } | ||
| if (formData.functions.inactive) { | ||
| addResource(AppwriteMigrationResource.Deployment); | ||
| } | ||
| if (formData.sites.root) { | ||
| addResource(AppwriteMigrationResource.Site); | ||
| addResource(AppwriteMigrationResource.SiteVariable); | ||
| } | ||
| if (formData.sites.inactive) { | ||
| addResource(AppwriteMigrationResource.SiteDeployment); | ||
| } |
Contributor
There was a problem hiding this comment.
Respect functions/sites env toggles when building resources.
Right now functions.env and sites.env are ignored, so environment variables are always included when the root toggle is on. This breaks user intent.
🐛 Proposed fix
if (formData.functions.root) {
addResource(AppwriteMigrationResource.Function);
- addResource(AppwriteMigrationResource.EnvironmentVariable);
}
+ if (formData.functions.env) {
+ addResource(AppwriteMigrationResource.EnvironmentVariable);
+ }
if (formData.functions.inactive) {
addResource(AppwriteMigrationResource.Deployment);
}
if (formData.sites.root) {
addResource(AppwriteMigrationResource.Site);
- addResource(AppwriteMigrationResource.SiteVariable);
}
+ if (formData.sites.env) {
+ addResource(AppwriteMigrationResource.SiteVariable);
+ }
if (formData.sites.inactive) {
addResource(AppwriteMigrationResource.SiteDeployment);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/stores/migration.ts` around lines 79 - 116, The code always adds
environment variable resources when functions.root or sites.root is true,
ignoring the specific env toggles; update the resource-building logic so
MigrationResource.EnvironmentVariable is only added when both
formData.functions.root and formData.functions.env are true (refer to
formData.functions and MigrationResource.EnvironmentVariable), and likewise only
add MigrationResource.SiteVariable when both formData.sites.root and
formData.sites.env are true (refer to formData.sites and
MigrationResource.SiteVariable); keep the existing
providerResources[provider].includes(...) guard and use the same
addResource(...) helper.
Update ResourcesFriendly map keys and AppwriteMigrationResource enum references to use kebab-case values (environment-variable, site-deployment, site-variable) matching the utopia-migration library.
Comment on lines
87
to
116
| if (formData.users.root) { | ||
| addResource(Resources.User); | ||
| addResource(AppwriteMigrationResource.User); | ||
| } | ||
| if (formData.databases.root) { | ||
| addResource(Resources.Database); | ||
| addResource(Resources.Table); | ||
| addResource(Resources.Column); | ||
| addResource(Resources.Index); | ||
| addResource(AppwriteMigrationResource.Database); | ||
| addResource(AppwriteMigrationResource.Table); | ||
| addResource(AppwriteMigrationResource.Column); | ||
| addResource(AppwriteMigrationResource.Index); | ||
| } | ||
| if (formData.databases.rows) { | ||
| addResource(Resources.Row); | ||
| addResource(AppwriteMigrationResource.Row); | ||
| } | ||
| if (formData.storage.root) { | ||
| addResource(Resources.Bucket); | ||
| addResource(Resources.File); | ||
| addResource(AppwriteMigrationResource.Bucket); | ||
| addResource(AppwriteMigrationResource.File); | ||
| } | ||
| if (formData.functions.root) { | ||
| addResource(AppwriteMigrationResource.Function); | ||
| addResource(AppwriteMigrationResource.Environmentvariable); | ||
| } | ||
| if (formData.functions.inactive) { | ||
| addResource(AppwriteMigrationResource.Deployment); | ||
| } | ||
| if (formData.sites.root) { | ||
| addResource(AppwriteMigrationResource.Site); | ||
| addResource(AppwriteMigrationResource.Sitevariable); | ||
| } | ||
| if (formData.sites.inactive) { | ||
| addResource(AppwriteMigrationResource.Sitedeployment); | ||
| } |
Contributor
There was a problem hiding this comment.
hardcodes AppwriteMigrationResource enum for all providers - only works if all provider enums share identical values
While the addResource helper filters via providerResources[provider].includes(resource), this assumes AppwriteMigrationResource.User === FirebaseMigrationResource.User etc. If the SDK defines distinct enum instances per provider, this will fail.
Safer approach: use provider-specific enums or string literals:
Suggested change
| if (formData.users.root) { | |
| addResource(Resources.User); | |
| addResource(AppwriteMigrationResource.User); | |
| } | |
| if (formData.databases.root) { | |
| addResource(Resources.Database); | |
| addResource(Resources.Table); | |
| addResource(Resources.Column); | |
| addResource(Resources.Index); | |
| addResource(AppwriteMigrationResource.Database); | |
| addResource(AppwriteMigrationResource.Table); | |
| addResource(AppwriteMigrationResource.Column); | |
| addResource(AppwriteMigrationResource.Index); | |
| } | |
| if (formData.databases.rows) { | |
| addResource(Resources.Row); | |
| addResource(AppwriteMigrationResource.Row); | |
| } | |
| if (formData.storage.root) { | |
| addResource(Resources.Bucket); | |
| addResource(Resources.File); | |
| addResource(AppwriteMigrationResource.Bucket); | |
| addResource(AppwriteMigrationResource.File); | |
| } | |
| if (formData.functions.root) { | |
| addResource(AppwriteMigrationResource.Function); | |
| addResource(AppwriteMigrationResource.Environmentvariable); | |
| } | |
| if (formData.functions.inactive) { | |
| addResource(AppwriteMigrationResource.Deployment); | |
| } | |
| if (formData.sites.root) { | |
| addResource(AppwriteMigrationResource.Site); | |
| addResource(AppwriteMigrationResource.Sitevariable); | |
| } | |
| if (formData.sites.inactive) { | |
| addResource(AppwriteMigrationResource.Sitedeployment); | |
| } | |
| if (formData.users.root) { | |
| addResource('user' as MigrationResource); | |
| } | |
| if (formData.databases.root) { | |
| addResource('database' as MigrationResource); | |
| addResource('table' as MigrationResource); | |
| addResource('column' as MigrationResource); | |
| addResource('index' as MigrationResource); | |
| } | |
| if (formData.databases.rows) { | |
| addResource('row' as MigrationResource); | |
| } | |
| if (formData.storage.root) { | |
| addResource('bucket' as MigrationResource); | |
| addResource('file' as MigrationResource); | |
| } | |
| if (formData.functions.root) { | |
| addResource('function' as MigrationResource); | |
| addResource('environment-variable' as MigrationResource); | |
| } | |
| if (formData.functions.inactive) { | |
| addResource('deployment' as MigrationResource); | |
| } | |
| if (formData.sites.root) { | |
| addResource('site' as MigrationResource); | |
| addResource('site-variable' as MigrationResource); | |
| } | |
| if (formData.sites.inactive) { | |
| addResource('site-deployment' as MigrationResource); | |
| } |
Comment on lines
+144
to
+178
| if (resources.includes(AppwriteMigrationResource.User)) { | ||
| formData.users.root = true; | ||
| } | ||
| if (resources.includes(Resources.Database)) { | ||
| if (resources.includes(AppwriteMigrationResource.Database)) { | ||
| formData.databases.root = true; | ||
| } | ||
| if (includesAll(resources, [Resources.Table, Resources.Column, Resources.Row] as Resources[])) { | ||
| if ( | ||
| includesAll(resources, [ | ||
| AppwriteMigrationResource.Table, | ||
| AppwriteMigrationResource.Column, | ||
| AppwriteMigrationResource.Row | ||
| ] as MigrationResource[]) | ||
| ) { | ||
| formData.databases.rows = true; | ||
| } | ||
| if (includesAll(resources, [Resources.Bucket, Resources.File] as Resources[])) { | ||
| if ( | ||
| includesAll(resources, [ | ||
| AppwriteMigrationResource.Bucket, | ||
| AppwriteMigrationResource.File | ||
| ] as MigrationResource[]) | ||
| ) { | ||
| formData.storage.root = true; | ||
| } | ||
| if (resources.includes(AppwriteMigrationResource.Function)) { | ||
| formData.functions.root = true; | ||
| } | ||
| if (resources.includes(AppwriteMigrationResource.Deployment)) { | ||
| formData.functions.inactive = true; | ||
| } | ||
| if (resources.includes(AppwriteMigrationResource.Site)) { | ||
| formData.sites.root = true; | ||
| } | ||
| if (resources.includes(AppwriteMigrationResource.Sitedeployment)) { | ||
| formData.sites.inactive = true; | ||
| } |
Contributor
There was a problem hiding this comment.
same hardcoded enum issue - assumes all provider enums have identical references
The resources array could contain Firebase/Supabase/NHost enum values, but code only checks AppwriteMigrationResource values.
Contributor
Additional Comments (1)
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/lib/stores/migration.ts (2)
167-177:⚠️ Potential issue | 🟠 MajorPopulate env toggles when converting resources back to form state.
EnvironmentvariableandSitevariableare not mapped toformData.functions.env/formData.sites.env, so UI toggles can render unchecked even when those resources are selected.🐛 Proposed fix
if (resources.includes(AppwriteMigrationResource.Function)) { formData.functions.root = true; } + if (resources.includes(AppwriteMigrationResource.Environmentvariable)) { + formData.functions.env = true; + } if (resources.includes(AppwriteMigrationResource.Deployment)) { formData.functions.inactive = true; } if (resources.includes(AppwriteMigrationResource.Site)) { formData.sites.root = true; } + if (resources.includes(AppwriteMigrationResource.Sitevariable)) { + formData.sites.env = true; + } if (resources.includes(AppwriteMigrationResource.Sitedeployment)) { formData.sites.inactive = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/stores/migration.ts` around lines 167 - 177, When converting selected resources back into form state in migration.ts, the code sets formData.functions.root/inactive and formData.sites.root/inactive but omits mapping AppwriteMigrationResource.Environmentvariable and AppwriteMigrationResource.Sitevariable; add checks like if (resources.includes(AppwriteMigrationResource.Environmentvariable)) { formData.functions.env = true } and if (resources.includes(AppwriteMigrationResource.Sitevariable)) { formData.sites.env = true } so the UI toggles for functions.env and sites.env reflect selected resources.
103-113:⚠️ Potential issue | 🟠 MajorRespect
functions.envandsites.envwhen building selected resources.Line 105 and Line 112 currently add env resources whenever
rootis enabled, so env opt-outs are ignored.🐛 Proposed fix
if (formData.functions.root) { addResource(AppwriteMigrationResource.Function); - addResource(AppwriteMigrationResource.Environmentvariable); + } + if (formData.functions.root && formData.functions.env) { + addResource(AppwriteMigrationResource.Environmentvariable); } if (formData.functions.inactive) { addResource(AppwriteMigrationResource.Deployment); } if (formData.sites.root) { addResource(AppwriteMigrationResource.Site); - addResource(AppwriteMigrationResource.Sitevariable); + } + if (formData.sites.root && formData.sites.env) { + addResource(AppwriteMigrationResource.Sitevariable); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/stores/migration.ts` around lines 103 - 113, The migration resource builder currently adds Environmentvariable and Sitevariable unconditionally when formData.functions.root or formData.sites.root are true, ignoring the user's env opt-out; update the logic in the block that calls addResource (e.g., where formData.functions.root and formData.sites.root are checked) to also check formData.functions.env and formData.sites.env respectively before calling addResource(AppwriteMigrationResource.Environmentvariable) and addResource(AppwriteMigrationResource.Sitevariable), so env resources are only added when the corresponding .env flag is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/stores/migration.ts`:
- Around line 167-177: When converting selected resources back into form state
in migration.ts, the code sets formData.functions.root/inactive and
formData.sites.root/inactive but omits mapping
AppwriteMigrationResource.Environmentvariable and
AppwriteMigrationResource.Sitevariable; add checks like if
(resources.includes(AppwriteMigrationResource.Environmentvariable)) {
formData.functions.env = true } and if
(resources.includes(AppwriteMigrationResource.Sitevariable)) {
formData.sites.env = true } so the UI toggles for functions.env and sites.env
reflect selected resources.
- Around line 103-113: The migration resource builder currently adds
Environmentvariable and Sitevariable unconditionally when
formData.functions.root or formData.sites.root are true, ignoring the user's env
opt-out; update the logic in the block that calls addResource (e.g., where
formData.functions.root and formData.sites.root are checked) to also check
formData.functions.env and formData.sites.env respectively before calling
addResource(AppwriteMigrationResource.Environmentvariable) and
addResource(AppwriteMigrationResource.Sitevariable), so env resources are only
added when the corresponding .env flag is true.
This reverts commit f4470c6.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/lib/stores/migration.ts (2)
103-113:⚠️ Potential issue | 🟠 MajorRespect
functions.env/sites.envwhen adding env resources.
EnvironmentVariableandSiteVariableare currently added whenever root is on, so the env checkboxes don’t control output. This breaks user intent.Suggested fix
if (formData.functions.root) { addResource(AppwriteMigrationResource.Function); - addResource(AppwriteMigrationResource.EnvironmentVariable); + } + if (formData.functions.env) { + addResource(AppwriteMigrationResource.EnvironmentVariable); } if (formData.functions.inactive) { addResource(AppwriteMigrationResource.Deployment); } if (formData.sites.root) { addResource(AppwriteMigrationResource.Site); - addResource(AppwriteMigrationResource.SiteVariable); + } + if (formData.sites.env) { + addResource(AppwriteMigrationResource.SiteVariable); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/stores/migration.ts` around lines 103 - 113, The code currently unconditionally adds env resources when formData.functions.root or formData.sites.root are true; change the logic in the block that calls addResource (look for addResource and AppwriteMigrationResource.EnvironmentVariable / AppwriteMigrationResource.SiteVariable) to also check formData.functions.env and formData.sites.env respectively before adding EnvironmentVariable or SiteVariable so those variables are only added when both the root and the corresponding env checkbox are enabled; leave existing additions of Function, Deployment, and Site unchanged.
167-178:⚠️ Potential issue | 🟠 MajorMap env resources back into form env toggles.
When hydrating form state,
EnvironmentVariableandSiteVariableare not mapped, so env toggles won’t reflect selected resources.Suggested fix
if (resources.includes(AppwriteMigrationResource.Function)) { formData.functions.root = true; } + if (resources.includes(AppwriteMigrationResource.EnvironmentVariable)) { + formData.functions.env = true; + } if (resources.includes(AppwriteMigrationResource.Deployment)) { formData.functions.inactive = true; } if (resources.includes(AppwriteMigrationResource.Site)) { formData.sites.root = true; } + if (resources.includes(AppwriteMigrationResource.SiteVariable)) { + formData.sites.env = true; + } if (resources.includes(AppwriteMigrationResource.SiteDeployment)) { formData.sites.inactive = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/stores/migration.ts` around lines 167 - 178, The hydration block currently maps Function/Deployment and Site/SiteDeployment but misses EnvironmentVariable and SiteVariable, so env toggles don't reflect selected resources; add checks for AppwriteMigrationResource.EnvironmentVariable and AppwriteMigrationResource.SiteVariable and set the corresponding formData env toggles (e.g., set formData.env.root = true for EnvironmentVariable and formData.env.inactive = true for SiteVariable) alongside the existing mappings in the same block so the form's env toggles reflect selected resources.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/stores/migration.ts`:
- Around line 103-113: The code currently unconditionally adds env resources
when formData.functions.root or formData.sites.root are true; change the logic
in the block that calls addResource (look for addResource and
AppwriteMigrationResource.EnvironmentVariable /
AppwriteMigrationResource.SiteVariable) to also check formData.functions.env and
formData.sites.env respectively before adding EnvironmentVariable or
SiteVariable so those variables are only added when both the root and the
corresponding env checkbox are enabled; leave existing additions of Function,
Deployment, and Site unchanged.
- Around line 167-178: The hydration block currently maps Function/Deployment
and Site/SiteDeployment but misses EnvironmentVariable and SiteVariable, so env
toggles don't reflect selected resources; add checks for
AppwriteMigrationResource.EnvironmentVariable and
AppwriteMigrationResource.SiteVariable and set the corresponding formData env
toggles (e.g., set formData.env.root = true for EnvironmentVariable and
formData.env.inactive = true for SiteVariable) alongside the existing mappings
in the same block so the form's env toggles reflect selected resources.
- Fix SDK enum casing (Environmentvariable, Sitedeployment, Sitevariable) - Use kebab-case keys in ResourcesFriendly to match backend values - Always include deployments when function/site root is checked - Remove environment variables checkbox from migration form - Add optional chaining for unknown resource types in details view - Sort migrations list by newest first
Comment on lines
+101
to
+105
| if (formData.functions.root) { | ||
| addResource(AppwriteMigrationResource.Function); | ||
| addResource(AppwriteMigrationResource.Environmentvariable); | ||
| addResource(AppwriteMigrationResource.Deployment); | ||
| } |
Contributor
There was a problem hiding this comment.
formData.functions.inactive checkbox is never checked - inactive deployments always included
The UI shows an "Include inactive deployments" checkbox for functions, but this code always adds Deployment when functions.root is true, regardless of the inactive checkbox state.
Either remove the checkbox from the UI or add conditional logic:
Suggested change
| if (formData.functions.root) { | |
| addResource(AppwriteMigrationResource.Function); | |
| addResource(AppwriteMigrationResource.Environmentvariable); | |
| addResource(AppwriteMigrationResource.Deployment); | |
| } | |
| if (formData.functions.root) { | |
| addResource(AppwriteMigrationResource.Function); | |
| addResource(AppwriteMigrationResource.Environmentvariable); | |
| if (formData.functions.inactive) { | |
| addResource(AppwriteMigrationResource.Deployment); | |
| } | |
| } |
Comment on lines
+106
to
110
| if (formData.sites.root) { | ||
| addResource(AppwriteMigrationResource.Site); | ||
| addResource(AppwriteMigrationResource.Sitevariable); | ||
| addResource(AppwriteMigrationResource.Sitedeployment); | ||
| } |
Contributor
There was a problem hiding this comment.
formData.sites.inactive checkbox is never checked - inactive deployments always included
Same issue as functions - the UI shows an "Include inactive deployments" checkbox for sites, but this code always adds Sitedeployment regardless of the checkbox state.
Suggested change
| if (formData.sites.root) { | |
| addResource(AppwriteMigrationResource.Site); | |
| addResource(AppwriteMigrationResource.Sitevariable); | |
| addResource(AppwriteMigrationResource.Sitedeployment); | |
| } | |
| if (formData.sites.root) { | |
| addResource(AppwriteMigrationResource.Site); | |
| addResource(AppwriteMigrationResource.Sitevariable); | |
| if (formData.sites.inactive) { | |
| addResource(AppwriteMigrationResource.Sitedeployment); | |
| } | |
| } |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/stores/migration.ts (1)
37-40:⚠️ Potential issue | 🟠 MajorAvoid shallow-copying
initialFormData(nested state gets mutated globally).Line 37 and Line 137 clone only the top-level object. Nested groups (
users,databases,functions,storage,sites) remain shared references, so writes inresourcesToMigrationFormcan mutate module defaults and breakreset()determinism.Proposed fix
+const cloneInitialFormData = (): MigrationFormData => ({ + users: { ...initialFormData.users }, + databases: { ...initialFormData.databases }, + functions: { ...initialFormData.functions }, + storage: { ...initialFormData.storage }, + sites: { ...initialFormData.sites } +}); + export const createMigrationFormStore = () => { - const store = writable({ ...initialFormData }); + const store = writable(cloneInitialFormData()); const reset = () => { - store.set({ ...initialFormData }); + store.set(cloneInitialFormData()); }; @@ export const resourcesToMigrationForm = (resources: MigrationResource[]): MigrationFormData => { - const formData = { ...initialFormData }; + const formData = cloneInitialFormData();Also applies to: 136-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/stores/migration.ts` around lines 37 - 40, The code shallow-copies initialFormData into the Svelte writable (store = writable({ ...initialFormData })) and in reset() (store.set({ ...initialFormData })), leaving nested objects (users, databases, functions, storage, sites) shared and vulnerable to mutation; replace the shallow clone with a deep clone (e.g., use structuredClone(initialFormData) or a deepClone utility) both when initializing store and inside reset(), and apply the same deep-clone fix to the other occurrence around resourcesToMigrationForm so nested state is isolated per-instance.
♻️ Duplicate comments (1)
src/lib/stores/migration.ts (1)
101-110:⚠️ Potential issue | 🟠 MajorInactive deployment toggles are currently ignored in serialization.
Line 104 and Line 109 always add deployment resources when root is enabled, but
formData.functions.inactive/formData.sites.inactiveare never read. This makes the inactive checkboxes effectively no-op.Proposed fix
if (formData.functions.root) { addResource(AppwriteMigrationResource.Function); addResource(AppwriteMigrationResource.Environmentvariable); - addResource(AppwriteMigrationResource.Deployment); + if (formData.functions.inactive) { + addResource(AppwriteMigrationResource.Deployment); + } } if (formData.sites.root) { addResource(AppwriteMigrationResource.Site); addResource(AppwriteMigrationResource.Sitevariable); - addResource(AppwriteMigrationResource.Sitedeployment); + if (formData.sites.inactive) { + addResource(AppwriteMigrationResource.Sitedeployment); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/stores/migration.ts` around lines 101 - 110, The code always adds Deployment and Sitedeployment when functions.root or sites.root is true, ignoring the inactive checkboxes; change the logic so addResource(AppwriteMigrationResource.Deployment) is only called when formData.functions.inactive is true (not just functions.root), and likewise call addResource(AppwriteMigrationResource.Sitedeployment) only when formData.sites.inactive is true; keep the existing additions for Function/Environmentvariable and Site/Sitevariable intact and only gate the deployment-related addResource calls on the corresponding .inactive flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/stores/migration.ts`:
- Around line 37-40: The code shallow-copies initialFormData into the Svelte
writable (store = writable({ ...initialFormData })) and in reset() (store.set({
...initialFormData })), leaving nested objects (users, databases, functions,
storage, sites) shared and vulnerable to mutation; replace the shallow clone
with a deep clone (e.g., use structuredClone(initialFormData) or a deepClone
utility) both when initializing store and inside reset(), and apply the same
deep-clone fix to the other occurrence around resourcesToMigrationForm so nested
state is isolated per-instance.
---
Duplicate comments:
In `@src/lib/stores/migration.ts`:
- Around line 101-110: The code always adds Deployment and Sitedeployment when
functions.root or sites.root is true, ignoring the inactive checkboxes; change
the logic so addResource(AppwriteMigrationResource.Deployment) is only called
when formData.functions.inactive is true (not just functions.root), and likewise
call addResource(AppwriteMigrationResource.Sitedeployment) only when
formData.sites.inactive is true; keep the existing additions for
Function/Environmentvariable and Site/Sitevariable intact and only gate the
deployment-related addResource calls on the corresponding .inactive flags.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/stores/migration.tssrc/routes/(console)/project-[region]-[project]/settings/migrations/(import)/importReport.sveltesrc/routes/(console)/project-[region]-[project]/settings/migrations/+page.tssrc/routes/(console)/project-[region]-[project]/settings/migrations/details.svelte
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.
Summary
Resourcesenum with provider-specific migration resource enums (AppwriteMigrationResource,FirebaseMigrationResource,NHostMigrationResource,SupabaseMigrationResource) from the SDKenvironment_variable,site_deployment,site_variable) to match SDK valuesTest plan
Summary by CodeRabbit
New Features
Bug Fixes / Improvements