(fix): address code review findings

- Fix positional args in cross-region and backup storage SDK calls - Escape shell metacharacters in connection commands - Fix $effect infinite retry loops in monitoring lazy-load sections - Add error notifications to silent monitoring loaders - Add previousId tracking to backups and branches $effect blocks - Use Type enum for backup type instead of string literals - Add missing Submit/Click analytics enum members - Replace raw string trackEvent calls with enum members - Use trackError for error analytics paths - Remove dead isShared code in dedicated overview - Fix database type string from 'dedicated' to 'dedicateddb'
2026-06-06 19:27:48 +00:00 · 2026-04-01 13:32:23 +13:00
parent 7a1aaf0e75
commit 2070784ba6
8 changed files with 77 additions and 116 deletions
@@ -152,10 +152,15 @@ export enum Click {
    DatabaseIndexDelete = 'click_index_delete',
    DatabaseTableDelete = 'click_table_delete',
    DatabaseRowDelete = 'click_row_delete',
+    DatabaseColdStart = 'click_database_cold_start',
    DatabaseDatabaseDelete = 'click_database_delete',
    DatabaseImportCsv = 'click_database_import_csv',
    DatabaseExportCsv = 'click_database_export_csv',
    DatabaseImportJson = 'click_database_import_json',
+    DatabasePause = 'click_database_pause',
+    DatabaseResume = 'click_database_resume',
+    DatabaseSpinDown = 'click_database_spin_down',
+    DedicatedMonitoringRefresh = 'click_dedicated_monitoring_refresh',
    DomainCreateClick = 'click_domain_create',
    DomainDeleteClick = 'click_domain_delete',
    DomainRetryDomainVerificationClick = 'click_domain_retry_domain_verification',
@@ -299,6 +304,10 @@ export enum Submit {
    DedicatedBackupCreate = 'submit_dedicated_backup_create',
    DedicatedBackupDelete = 'submit_dedicated_backup_delete',
    DedicatedBackupRestore = 'submit_dedicated_backup_restore',
+    DedicatedBackupVerify = 'submit_dedicated_backup_verify',
+    DedicatedBranchCreate = 'submit_dedicated_branch_create',
+    DedicatedBranchDelete = 'submit_dedicated_branch_delete',
+    DedicatedDatabaseMigrate = 'submit_dedicated_database_migrate',
    DedicatedPitrRestore = 'submit_dedicated_pitr_restore',
    DatabaseInstallExtension = 'submit_database_install_extension',
    DatabaseUninstallExtension = 'submit_database_uninstall_extension',
@@ -57,7 +57,7 @@
    let pitrTargetDateTime = $state('');

    let showCreateBackupModal = $state(false);
-    let backupType = $state<'full' | 'incremental'>('full');
+    let backupType = $state<Type>(Type.Full);

    let verifyingBackupId = $state<string | null>(null);
    let showVerifyConfirm = $state(false);
@@ -167,7 +167,7 @@
        try {
            await computeSdk.createDatabaseBackup({
                databaseId: database.$id,
-                type: backupType as unknown as Type
+                type: backupType
            });
            addNotification({
                type: 'success',
@@ -175,7 +175,7 @@
            });
            trackEvent(Submit.DedicatedBackupCreate);
            showCreateBackupModal = false;
-            backupType = 'full';
+            backupType = Type.Full;
            await loadBackups();
        } catch (error) {
            addNotification({
@@ -281,7 +281,7 @@
                type: 'success',
                message: 'Backup verification started'
            });
-            trackEvent('submit_dedicated_backup_verify');
+            trackEvent(Submit.DedicatedBackupVerify);
            showVerifyConfirm = false;
            verifyBackup = null;
            await loadBackups();
@@ -290,7 +290,7 @@
                type: 'error',
                message: error.message
            });
-            trackEvent('submit_dedicated_backup_verify_error');
+            trackError(error, Submit.DedicatedBackupVerify);
        } finally {
            verifyingBackupId = null;
        }
@@ -308,11 +308,15 @@
        return `${date.getFullYear()}-${pad(date.getMonth() + 1)}-${pad(date.getDate())}T${pad(date.getHours())}:${pad(date.getMinutes())}`;
    }

-    // Load data on mount
+    let previousId = $state('');
+
    $effect(() => {
-        loadBackups();
-        loadRestorations();
-        loadPitrWindows();
+        if (database?.$id && database.$id !== previousId) {
+            previousId = database.$id;
+            loadBackups();
+            loadRestorations();
+            loadPitrWindows();
+        }
    });
 </script>

@@ -734,13 +738,13 @@
                id="backup-full"
                name="backupType"
                label="Full backup"
-                value="full"
+                value={Type.Full}
                bind:group={backupType} />
            <InputRadio
                id="backup-incremental"
                name="backupType"
                label="Incremental backup (only changes since last full backup)"
-                value="incremental"
+                value={Type.Incremental}
                bind:group={backupType} />
        </Layout.Stack>
    </Layout.Stack>
@@ -6,7 +6,7 @@
    import { toLocaleDateTime } from '$lib/helpers/date';
    import { addNotification } from '$lib/stores/notifications';
    import { sdk } from '$lib/stores/sdk';
-    import { trackEvent } from '$lib/actions/analytics';
+    import { Submit, trackEvent, trackError } from '$lib/actions/analytics';
    import { ID, type Models } from '@appwrite.io/console';
    import {
        ActionMenu,
@@ -69,14 +69,14 @@
                type: 'success',
                message: 'Branch created'
            });
-            trackEvent('submit_dedicated_branch_create');
+            trackEvent(Submit.DedicatedBranchCreate);
            await loadBranches();
        } catch (error) {
            addNotification({
                type: 'error',
                message: error.message
            });
-            trackEvent('submit_dedicated_branch_create_error');
+            trackError(error, Submit.DedicatedBranchCreate);
        } finally {
            isCreating = false;
        }
@@ -93,7 +93,7 @@
                type: 'success',
                message: 'Branch deleted'
            });
-            trackEvent('submit_dedicated_branch_delete');
+            trackEvent(Submit.DedicatedBranchDelete);
            showDeleteConfirm = false;
            selectedBranch = null;
            await loadBranches();
@@ -102,7 +102,7 @@
                type: 'error',
                message: error.message
            });
-            trackEvent('submit_dedicated_branch_delete_error');
+            trackError(error, Submit.DedicatedBranchDelete);
        }
    }

@@ -111,8 +111,11 @@
        return toLocaleDateTime(new Date(timestamp * 1000).toISOString());
    }

+    let previousId = $state('');
+
    $effect(() => {
-        if (database) {
+        if (database?.$id && database.$id !== previousId) {
+            previousId = database.$id;
            loadBranches();
        }
    });
@@ -8,7 +8,7 @@
    import { addNotification } from '$lib/stores/notifications';
    import { sdk } from '$lib/stores/sdk';
    import { Dependencies } from '$lib/constants';
-    import { trackEvent } from '$lib/actions/analytics';
+    import { Click, trackEvent } from '$lib/actions/analytics';
    import { capitalize } from '$lib/helpers/string';
    import { Status as DatabaseStatus, type Models } from '@appwrite.io/console';
    import {
@@ -68,16 +68,11 @@
    let isColdStarting = $state(false);
    let isPausing = $state(false);
    let isResuming = $state(false);
-    let isSpinningDown = $state(false);
    let connectionTab = $state<'direct' | 'string'>('direct');

    const isDedicated = $derived(database.type === 'dedicateddb');
-    const isShared = $derived(false);
    const isActive = $derived(database.status === 'ready' || database.status === 'active');
    const isPaused = $derived(database.status === 'paused');
-    const containerIsRunning = $derived(
-        database.containerStatus === 'running' || database.containerStatus === 'active'
-    );

    // Map database status to Status component status
    const statusComponentStatus = $derived.by((): 'ready' | 'processing' | 'failed' | 'pending' => {
@@ -176,7 +171,7 @@
                message: 'Database is starting up'
            });

-            trackEvent('click_database_cold_start');
+            trackEvent(Click.DatabaseColdStart);

            await invalidate(Dependencies.DATABASE);
        } catch (error) {
@@ -201,7 +196,7 @@
                type: 'success',
                message: 'Database is pausing'
            });
-            trackEvent('click_database_pause');
+            trackEvent(Click.DatabasePause);
            await invalidate(Dependencies.DATABASE);
        } catch (error) {
            addNotification({
@@ -225,7 +220,7 @@
                type: 'success',
                message: 'Database is resuming'
            });
-            trackEvent('click_database_resume');
+            trackEvent(Click.DatabaseResume);
            await invalidate(Dependencies.DATABASE);
        } catch (error) {
            addNotification({
@@ -237,30 +232,6 @@
        }
    }

-    async function spinDownDatabase() {
-        isSpinningDown = true;
-        try {
-            await sdk.forProject(page.params.region, page.params.project).compute.updateDatabase({
-                databaseId: database.$id,
-                status: 'inactive' as unknown as DatabaseStatus
-            });
-
-            addNotification({
-                type: 'success',
-                message: 'Database container is spinning down'
-            });
-            trackEvent('click_database_spin_down');
-            await invalidate(Dependencies.DATABASE);
-        } catch (error) {
-            addNotification({
-                type: 'error',
-                message: error.message
-            });
-        } finally {
-            isSpinningDown = false;
-        }
-    }
-
    // Check if connection details are available
    const hasConnectionDetails = $derived(!!database.hostname || !!database.connectionString);

@@ -277,15 +248,19 @@
        return `${database.engine}://${user}:${password}@${database.hostname}:${database.connectionPort}${suffix}`;
    });

+    function escapeShellSingleQuote(value: string): string {
+        return value.replace(/'/g, "'\\''");
+    }
+
    function getConnectionCommand(): string {
        if (!resolvedConnectionString) return '';

        switch (database.engine) {
            case 'postgres':
-                return `psql "${resolvedConnectionString}"`;
+                return `psql '${escapeShellSingleQuote(resolvedConnectionString)}'`;
            case 'mysql':
            case 'mariadb':
-                return `mysql -h ${database.hostname} -P ${database.connectionPort} -u ${database.connectionUser} -p'${database.connectionPassword}'`;
+                return `mysql -h '${escapeShellSingleQuote(database.hostname)}' -P '${escapeShellSingleQuote(String(database.connectionPort))}' -u '${escapeShellSingleQuote(database.connectionUser)}' -p'${escapeShellSingleQuote(database.connectionPassword)}'`;
            default:
                return resolvedConnectionString;
        }
@@ -405,11 +380,6 @@
                    {isResuming ? 'Resuming...' : 'Resume'}
                </Button>
            {/if}
-            {#if isShared && isActive && containerIsRunning}
-                <Button secondary disabled={isSpinningDown} on:click={spinDownDatabase}>
-                    {isSpinningDown ? 'Spinning down...' : 'Spin Down'}
-                </Button>
-            {/if}
            <Button secondary disabled={isRefreshing} on:click={refreshStatus}>
                <Icon icon={IconRefresh} size="s" slot="start" />
                Refresh
@@ -523,48 +493,6 @@
        </CardGrid>
    {/if}

-    <!-- Free Tier Limits (shared databases only) -->
-    {#if isShared}
-        <CardGrid>
-            <svelte:fragment slot="title">Free Tier Limits</svelte:fragment>
-            Your shared database runs within the free tier. Resources are constrained to the limits below.
-            Upgrade to a dedicated database for higher limits.
-            <svelte:fragment slot="aside">
-                <Layout.Grid columns={2} columnsS={1} gap="l">
-                    <Layout.Stack gap="xxs">
-                        <Typography.Caption variant="400" color="--fgcolor-neutral-tertiary">
-                            Storage
-                        </Typography.Caption>
-                        <Typography.Text variant="m-500">1 GB</Typography.Text>
-                    </Layout.Stack>
-                    <Layout.Stack gap="xxs">
-                        <Typography.Caption variant="400" color="--fgcolor-neutral-tertiary">
-                            Max Connections
-                        </Typography.Caption>
-                        <Typography.Text variant="m-500">10</Typography.Text>
-                    </Layout.Stack>
-                    <Layout.Stack gap="xxs">
-                        <Typography.Caption variant="400" color="--fgcolor-neutral-tertiary">
-                            Query Timeout
-                        </Typography.Caption>
-                        <Typography.Text variant="m-500">15s</Typography.Text>
-                    </Layout.Stack>
-                    <Layout.Stack gap="xxs">
-                        <Typography.Caption variant="400" color="--fgcolor-neutral-tertiary">
-                            Idle Timeout
-                        </Typography.Caption>
-                        <Typography.Text variant="m-500">
-                            15 min
-                            <Typography.Caption variant="400" color="--fgcolor-neutral-tertiary">
-                                (scales to zero)
-                            </Typography.Caption>
-                        </Typography.Text>
-                    </Layout.Stack>
-                </Layout.Grid>
-            </svelte:fragment>
-        </CardGrid>
-    {/if}
-
    <!-- Resources -->
    <CardGrid>
        <svelte:fragment slot="title">Resources</svelte:fragment>
@@ -6,7 +6,7 @@
    import { sdk } from '$lib/stores/sdk';
    import { calculateSize } from '$lib/helpers/sizeConvertion';
    import { toLocaleDateTime } from '$lib/helpers/date';
-    import { trackEvent } from '$lib/actions/analytics';
+    import { Click, trackEvent } from '$lib/actions/analytics';
    import { type Models } from '@appwrite.io/console';
    import { Period } from '$lib/sdk/dedicated';
    import {
@@ -141,6 +141,10 @@
            slowQueries = await computeSdk.listDatabaseQueries({ databaseId: database.$id });
        } catch (error) {
            slowQueries = { total: 0, slowQueries: [] };
+            addNotification({
+                type: 'error',
+                message: `Failed to load slow queries: ${error.message}`
+            });
        } finally {
            isLoadingSlowQueries = false;
        }
@@ -155,6 +159,10 @@
            });
        } catch (error) {
            performanceInsights = null;
+            addNotification({
+                type: 'error',
+                message: `Failed to load performance insights: ${error.message}`
+            });
        } finally {
            isLoadingInsights = false;
        }
@@ -167,6 +175,10 @@
            auditLogs = await computeSdk.listDatabaseLogs({ databaseId: database.$id });
        } catch (error) {
            auditLogs = { total: 0, auditLogs: [] };
+            addNotification({
+                type: 'error',
+                message: `Failed to load audit logs: ${error.message}`
+            });
        } finally {
            isLoadingAuditLogs = false;
        }
@@ -182,6 +194,7 @@
            schemaLoaded = true;
        } catch (error) {
            schema = null;
+            schemaLoaded = true;
            addNotification({
                type: 'error',
                message: `Failed to load schema: ${error.message}`
@@ -240,6 +253,7 @@
            tuningLoaded = true;
        } catch (error) {
            tuningResult = null;
+            tuningLoaded = true;
            addNotification({
                type: 'error',
                message: `Failed to load tuning recommendations: ${error.message}`
@@ -259,6 +273,7 @@
            indexSuggestionsLoaded = true;
        } catch (error) {
            indexSuggestions = null;
+            indexSuggestionsLoaded = true;
            addNotification({
                type: 'error',
                message: `Failed to load index suggestions: ${error.message}`
@@ -269,7 +284,7 @@
    }

    async function refreshAll() {
-        trackEvent('dedicated_monitoring_refresh');
+        trackEvent(Click.DedicatedMonitoringRefresh);
        await Promise.all([
            loadMetrics(),
            loadSlowQueries(),
@@ -1,7 +1,7 @@
 <script lang="ts">
    import { invalidate } from '$app/navigation';
    import { page } from '$app/state';
-    import { trackEvent } from '$lib/actions/analytics';
+    import { Submit, trackEvent, trackError } from '$lib/actions/analytics';
    import { CardGrid, Confirm } from '$lib/components';
    import { Dependencies } from '$lib/constants';
    import { Button } from '$lib/elements/forms';
@@ -17,11 +17,11 @@
        database: Models.DedicatedDatabase;
    } = $props();

-    const currentType = $derived(database.type === 'dedicated' ? 'Dedicated' : 'Shared');
+    const currentType = $derived(database.type === 'dedicateddb' ? 'Dedicated' : 'Shared');
    const targetType = $derived(
-        database.type === 'dedicated' ? TargetType.Shared : TargetType.Dedicated
+        database.type === 'dedicateddb' ? TargetType.Shared : TargetType.Dedicated
    );
-    const targetLabel = $derived(database.type === 'dedicated' ? 'Shared' : 'Dedicated');
+    const targetLabel = $derived(database.type === 'dedicateddb' ? 'Shared' : 'Dedicated');

    let showConfirm = $state(false);

@@ -41,14 +41,14 @@
                type: 'success'
            });

-            trackEvent('submit_dedicated_database_migrate');
+            trackEvent(Submit.DedicatedDatabaseMigrate);
            showConfirm = false;
        } catch (error) {
            addNotification({
                message: error.message,
                type: 'error'
            });
-            trackEvent('submit_dedicated_database_migrate_error');
+            trackError(error, Submit.DedicatedDatabaseMigrate);
        }
    }
 </script>
@@ -73,7 +73,7 @@
                </Layout.Stack>
            </Layout.Grid>
            <Alert.Inline status="info" title="Migration details">
-                {#if database.type === 'dedicated'}
+                {#if database.type === 'dedicateddb'}
                    Migrating to shared converts your database to a serverless pod that scales to
                    zero when idle, reducing costs for low-traffic workloads.
                {:else}
@@ -45,7 +45,7 @@
    onMount(async () => {
        try {
            const projectSdk = sdk.forProject(page.params.region, page.params.project);
-            config = await projectSdk.compute.getBackupStorageConfig(database.$id);
+            config = await projectSdk.compute.getBackupStorageConfig({ databaseId: database.$id });
            isConfigured = true;
        } catch {
            // 404 means not configured
@@ -101,7 +101,7 @@
        isRemoving = true;
        try {
            const projectSdk = sdk.forProject(page.params.region, page.params.project);
-            await projectSdk.compute.deleteBackupStorageConfig(database.$id);
+            await projectSdk.compute.deleteBackupStorageConfig({ databaseId: database.$id });

            isConfigured = false;
            config = null;
@@ -72,7 +72,9 @@
    onMount(async () => {
        try {
            const projectSdk = sdk.forProject(page.params.region, page.params.project);
-            crossRegionStatus = await projectSdk.compute.getCrossRegionStatus(database.$id);
+            crossRegionStatus = await projectSdk.compute.getCrossRegionStatus({
+                databaseId: database.$id
+            });
            isEnabled = crossRegionStatus.enabled;
        } catch {
            // 404 means not enabled
@@ -88,10 +90,10 @@
        isEnabling = true;
        try {
            const projectSdk = sdk.forProject(page.params.region, page.params.project);
-            crossRegionStatus = await projectSdk.compute.enableCrossRegion(
-                database.$id,
+            crossRegionStatus = await projectSdk.compute.enableCrossRegion({
+                databaseId: database.$id,
                standbyRegion
-            );
+            });

            isEnabled = true;
            standbyRegion = '';
@@ -119,7 +121,7 @@
        isDisabling = true;
        try {
            const projectSdk = sdk.forProject(page.params.region, page.params.project);
-            await projectSdk.compute.disableCrossRegion(database.$id);
+            await projectSdk.compute.disableCrossRegion({ databaseId: database.$id });

            isEnabled = false;
            crossRegionStatus = null;
@@ -148,7 +150,7 @@
        isFailingOver = true;
        try {
            const projectSdk = sdk.forProject(page.params.region, page.params.project);
-            await projectSdk.compute.triggerCrossRegionFailover(database.$id);
+            await projectSdk.compute.triggerCrossRegionFailover({ databaseId: database.$id });

            showFailoverConfirm = false;