refactor: database container logic #21
Conversation
- Removed legacy container creation and update logic from useContainerActions hook. - Updated DatabaseSelectionForm to ensure providers are registered. - Deleted obsolete database form schema. - Refactored container-configuration-step to utilize dynamic fields from providers. - Enhanced review-step to generate Docker command using provider logic. - Replaced useContainerEdit with useDatabaseEditWizard for editing containers. - Implemented provider-based validation and transformation for container updates. - Removed outdated container settings and request types from shared types. - Cleaned up container utility functions to align with new provider architecture.
…nd implement hooks for database actions and list management
…mline Docker status handling
…management system
…lds for database providers
…and port fields into dynamic section
…nd update dynamic form fields accordingly
…ock implementations
There was a problem hiding this comment.
Pull Request Overview
This refactor introduces a provider-based architecture for database container management, replacing the previously hardcoded database-specific logic with a flexible system that supports extensibility and maintainability.
Key changes:
- Replaced hardcoded database configurations with a pluggable provider system
- Unified Docker container operations using generic
DockerRunRequeststructures - Migrated from Zod validation to provider-based field-level validation
- Removed legacy utility functions in favor of provider-generated configurations
Reviewed Changes
Copilot reviewed 67 out of 68 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/test/utils/test-utils.tsx |
Added mock factory functions for testing the new provider system |
src/test/utils/mock-providers.ts |
Created mock database provider for comprehensive testing |
src/test/unit/providers.test.ts |
Comprehensive test suite for all database providers |
src/test/unit/database-registry.test.ts |
Tests for the new provider registry system |
src/shared/utils/docker.ts |
Removed legacy Docker utility functions (replaced by providers) |
src/shared/utils/container.ts |
Removed legacy container utilities (replaced by providers) |
src/shared/types/settings.ts |
Removed legacy settings types (replaced by provider configurations) |
src/shared/types/container.ts |
Simplified container types, removed legacy request structures |
src/shared/components/ui/checkbox.tsx |
Added checkbox UI component for form rendering |
src/shared/components/DatabaseConfigPanel.tsx |
Removed legacy configuration panel (replaced by provider-based forms) |
src/pages/main/hooks/use-main-page.ts |
Simplified to use new database actions API |
src/pages/main/MainPage.tsx |
Removed legacy config panel usage |
src/pages/edit-container/hooks/use-database-edit-wizard.ts |
New provider-based container editing logic |
src/pages/edit-container/hooks/use-container-edit.ts |
Removed legacy container editing logic |
src/pages/edit-container/components/EditContainerForm.tsx |
Refactored to use dynamic provider-based forms |
src/pages/create-container/steps/review-step.tsx |
Updated to use provider-generated Docker commands |
src/pages/create-container/steps/database-selection-step.tsx |
Updated to use provider registry for database options |
src/pages/create-container/steps/container-configuration-step.tsx |
Simplified to use dynamic provider-based form sections |
src/pages/create-container/schemas/database-form.schema.ts |
Removed Zod schemas (replaced by provider validation) |
src/pages/create-container/hooks/use-container-creation-wizard.ts |
Refactored to use provider system with field-level validation |
src/pages/create-container/components/DatabaseSelectionForm.tsx |
Updated to ensure provider registration |
src/features/docker/hooks/use-docker-status.ts |
Simplified Docker status checking |
src/features/databases/types/form.types.ts |
New type definitions for dynamic form generation |
src/features/databases/types/docker.types.ts |
New generic Docker configuration types |
src/features/databases/registry/database-registry.ts |
Central registry for database providers |
src/features/databases/registry/database-provider.interface.ts |
Interface defining provider contract |
src/features/databases/providers/redis.provider.tsx |
Redis-specific provider implementation |
src/features/databases/providers/postgres.provider.tsx |
PostgreSQL-specific provider implementation |
src/features/databases/providers/mysql.provider.tsx |
MySQL-specific provider implementation |
src/features/databases/providers/mongodb.provider.tsx |
MongoDB-specific provider implementation |
src/features/databases/providers/index.ts |
Auto-registration of all providers |
src/features/databases/hooks/use-database-list.ts |
Renamed from container list hooks |
src/features/databases/hooks/use-database-actions.ts |
Database-specific action hooks |
src/features/databases/components/dynamic-form-section.tsx |
Dynamic form rendering for provider fields |
src/features/databases/components/dynamic-form-field.tsx |
Individual field rendering for provider configurations |
src/features/databases/api/databases.api.ts |
Unified API for database container operations |
…ks for provider access
…reSQL, and Redis to include latest releases
✅ Actions performedFull review triggered. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMove from per-database static flows to a provider-driven DockerRunRequest model: add database providers and registry, new Docker types, update Tauri commands and Docker service, migrate frontend to dynamic provider-driven forms and hooks, add provider and Docker integration tests, and remove many legacy container utilities, hooks, tests, and docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Frontend (provider form)
participant Registry as DatabaseRegistry
participant API as databasesApi
participant Tauri as Tauri backend
participant Svc as DockerService
participant Docker as Docker daemon
User->>UI: fills and submits provider form
UI->>Registry: get(providerId)
Registry-->>UI: Provider
UI->>UI: provider.buildDockerArgs(config) → DockerRunArgs
UI->>API: databasesApi.create(DockerRunRequest)
API->>Tauri: invoke(create_container_from_docker_args, request)
Tauri->>Svc: build_docker_command_from_args(name, docker_args)
Svc-->>Tauri: docker run args (Vec<String>)
Tauri->>Docker: docker run ...
Docker-->>Tauri: container id / status
Tauri-->>API: DatabaseContainer JSON
API-->>UI: created Container
UI-->>User: show created container
sequenceDiagram
autonumber
participant UI as Edit Form
participant API as databasesApi
participant Tauri as Tauri backend
participant Store as In-memory/persistent store
participant Docker as Docker daemon
UI->>API: databasesApi.update(containerId, DockerRunRequest)
API->>Tauri: invoke(update_container_from_docker_args, containerId, request)
alt recreation required (name/port/persistData change)
Tauri->>Docker: stop & remove old container
Tauri->>Docker: create/migrate/remove volumes
Tauri->>Docker: docker run new container
Docker-->>Tauri: new container id/status
Tauri->>Store: persist new container metadata
else metadata-only update
Tauri->>Store: update metadata only
end
Tauri-->>API: updated container JSON
API-->>UI: updated container
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/shared/types/container.ts (2)
24-26: Do not persist plaintext credentials in ContainerComment says “stored and displayed”. Persisting password is a security risk (secrets exposure). Store secrets in OS keychain/secure store, or only indicate presence.
- username?: string; - password?: string; + username?: string; + // Do not persist plaintext passwords. Prefer secret reference or presence flag. + hasPassword?: boolean;Follow up: scrub any persistence/logging of passwords and migrate storage accordingly.
21-21: Use a serializable type for createdAtDate objects don’t survive JSON (become strings). Prefer ISO string.
- createdAt: Date; + createdAt: string; // ISO 8601If consumers need Date, parse on read.
src/pages/create-container/steps/review-step.tsx (1)
37-94: Quote env var values and harden against provider errorsUnescaped values (spaces, $, quotes) will break copy/paste. Also, a provider throwing will crash the render. Add shell-escape and wrap in try/catch.
-/** - * Generate Docker command preview from form data using provider - */ -function generateDockerCommand(formData: CreateDatabaseFormData): string { - const { databaseSelection, containerConfiguration } = formData; - const { dbType } = databaseSelection; - - if (!dbType) { - return '# Please select a database type first'; - } - - // Get the provider for this database type - const provider = databaseRegistry.get(dbType); - if (!provider) { - return `# Error: No provider found for ${dbType}`; - } - - // Let the provider build the Docker arguments - const dockerArgs = provider.buildDockerArgs(containerConfiguration); - const { name } = containerConfiguration; - - // Format the command nicely for display - const lines: string[] = ['docker run -d \\']; - - // Container name - lines.push(` --name ${name} \\`); +/** + * Generate Docker command preview from form data using provider + */ +function generateDockerCommand(formData: CreateDatabaseFormData): string { + const { databaseSelection, containerConfiguration } = formData; + const { dbType } = databaseSelection; + + if (!dbType) { + return '# Please select a database type first'; + } + + // Get the provider for this database type + const provider = databaseRegistry.get(dbType); + if (!provider) { + return `# Error: No provider found for ${dbType}`; + } + + // Minimal POSIX shell arg escaper for values + const shellEscape = (val: unknown) => { + const s = String(val ?? ''); + if (s === '') return "''"; + return `'${s.replace(/'/g, `'\\''`)}'`; + }; + + try { + // Let the provider build the Docker arguments + const dockerArgs = provider.buildDockerArgs(containerConfiguration); + const { name } = containerConfiguration; + + // Format the command nicely for display + const lines: string[] = ['docker run -d \\']; + + // Container name + lines.push(` --name ${name} \\`); @@ - // Environment variables - if (dockerArgs.envVars && Object.keys(dockerArgs.envVars).length > 0) { - for (const [key, value] of Object.entries(dockerArgs.envVars)) { - lines.push(` -e ${key}=${value} \\`); - } - } + // Environment variables + if (dockerArgs.envVars && Object.keys(dockerArgs.envVars).length > 0) { + for (const [key, value] of Object.entries(dockerArgs.envVars)) { + lines.push(` -e ${key}=${shellEscape(value)} \\`); + } + } @@ - return lines.join('\n'); + return lines.join('\n'); + } catch (err) { + const message = + (err as Error)?.message ?? (typeof err === 'string' ? err : 'unknown error'); + return `# Error generating command: ${message}`; + } }
♻️ Duplicate comments (2)
src/pages/create-container/steps/database-selection-step.tsx (1)
113-113: Fallback when provider.icon is nullRender a fallback (e.g., first letter badge) to avoid empty avatars.
- {provider.icon} + {provider.icon ?? ( + <span className="text-sm font-semibold text-white"> + {provider.name?.charAt(0) ?? '?'} + </span> + )}src/features/databases/providers/redis.provider.tsx (1)
26-63: Centralize Redis version tags in a single sourceAvoid hard-coding tags in each provider. Extract to shared constants/config to ease updates and reuse across UI and validation.
🧹 Nitpick comments (32)
src/shared/components/ui/checkbox.tsx (1)
14-14: Optional: Minor redundancy in dark mode styling.The
dark:data-[state=checked]:bg-primaryclass appears redundant sincedata-[state=checked]:bg-primaryalready applies the primary background in checked state without a dark-specific variant. The dark mode would inherit the samebg-primaryvalue.This is likely intentional for explicitness or future-proofing, but you could simplify if the values are identical.
src-tauri/tests/unit/docker_service_test.rs (1)
30-45: Fragile test assertions—verify command structure, not concatenated strings.Joining command arguments into a string and using
contains()makes these tests brittle. Argument order matters when concatenating, so a semantically correct reordering of flags would break the test.Consider asserting on the
Vec<String>structure directly:#[test] fn test_build_docker_command_from_args_basic() { let service = DockerService::new(); let args = create_test_docker_args(); let command_args = service.build_docker_command_from_args("test-postgres", &args); - let command = command_args.join(" "); - - // Verify basic structure - assert!(command.contains("run")); - assert!(command.contains("-d")); - assert!(command.contains("--name")); - assert!(command.contains("test-postgres")); - assert!(command.contains("postgres:16")); + // Verify required arguments are present + assert!(command_args.contains(&"run".to_string())); + assert!(command_args.contains(&"-d".to_string())); + assert!(command_args.contains(&"--name".to_string())); + assert!(command_args.contains(&"test-postgres".to_string())); + assert!(command_args.contains(&"postgres:16".to_string())); + + // Verify argument positioning if order matters + assert_eq!(command_args[0], "run"); + assert_eq!(command_args.last(), Some(&"postgres:16".to_string())); }Apply similar changes to the other test functions (lines 47-156).
src/features/databases/registry/database-registry.ts (1)
7-50: Document the registry's immutability.The
DatabaseRegistryclass is not exported, and the registry is immutable after construction (noaddorremovemethods). This design choice prevents runtime modification of providers, which may be intentional for stability.Consider adding a JSDoc comment to clarify this behavior:
+/** + * Internal registry class for managing database providers. + * Note: The registry is immutable after construction. Providers cannot be + * added or removed at runtime. To modify the provider list, update the + * createDatabaseRegistry factory function. + */ class DatabaseRegistry { private providers = new Map<string, DatabaseProvider>();src/features/databases/components/dynamic-form-field.tsx (2)
30-31: Robustly join nested field namesSimple concatenation risks missing/double dots. Join safely.
- const fullFieldName = fieldPrefix + field.name; + const fullFieldName = [fieldPrefix, field.name].filter(Boolean).join('.');
107-114: Improve a11y: wire aria-invalid/aria-describedby to error textHook inputs/select to the inline error for screen readers.
<Input {...controllerField} id={fullFieldName} type={field.type} placeholder={field.placeholder} readOnly={field.readonly} - className={error ? 'border-destructive' : ''} + className={error ? 'border-destructive' : ''} + aria-invalid={!!error} + aria-describedby={error ? `${fullFieldName}-error` : undefined} /><SelectTrigger id={fullFieldName} - className={error ? 'border-destructive' : ''} + className={error ? 'border-destructive' : ''} + aria-invalid={!!error} + aria-describedby={error ? `${fullFieldName}-error` : undefined} >-{error && ( - <p className="text-xs text-destructive">{error.message as string}</p> -)} +{error && ( + <p id={`${fullFieldName}-error`} className="text-xs text-destructive"> + {error.message as string} + </p> +)}Also applies to: 141-146, 181-183
src/shared/types/container.ts (1)
22-23: Provider-specific fields should be optional or moved to metadatamaxConnections/persistData/enableAuth may not apply to all db types. Consider optional or a metadata map to avoid sparse models.
Also applies to: 27-28
src-tauri/src/services/docker.rs (1)
65-106: Enrich docker run args: restart policy, labels, and future-proofingCommand builds fine. For resilience/management, add restart policy and identifying label. Consider protocol/host IP and volume mode in types.
// Add environment variables for (key, value) in &docker_args.env_vars { args.push("-e".to_string()); args.push(format!("{}={}", key, value)); } + // Add restart policy (optional) + if let Some(policy) = &docker_args.restart_policy { + args.push("--restart".to_string()); + args.push(policy.clone()); // e.g., "unless-stopped" + } + + // Tag containers created by this app for discovery/cleanup + args.push("--label".to_string()); + args.push("com.docker-db-manager=1".to_string()); + // Add image args.push(docker_args.image.clone());Additionally:
- Ports: allow host_ip/protocol (e.g., 127.0.0.1:host:container/tcp).
- Volumes: support mode (ro/rw). Update DockerRunArgs accordingly.
src/features/databases/types/form.types.ts (2)
5-13: Align naming with DOM: prefer readOnly over readonlyAvoid confusion with TS’s readonly keyword; match HTML prop for consistency with components.
-export interface FormFieldBase { +export interface FormFieldBase { name: string; label: string; placeholder?: string; required?: boolean; - readonly?: boolean; + readOnly?: boolean; helpText?: string; defaultValue?: string | number | boolean; }And in Checkbox/Text field usage/sites, switch to field.readOnly. Pair with component changes.
Also applies to: 30-33
15-23: Pattern is defined here; ensure UI layer enforces itDynamicFormField currently ignores validation.pattern. Implement a RHF pattern rule (see related comment in component).
src/test/utils/test-utils.tsx (2)
73-82: Avoid truthiness for volume fields; check types/non-empty stringsEmpty strings are invalid, but check explicitly rather than truthiness mixed with typeof.
- for (const volume of args.volumes) { - if ( - !volume.name || - !volume.path || - typeof volume.name !== 'string' || - typeof volume.path !== 'string' - ) { - return false; - } - } + for (const volume of args.volumes) { + if ( + typeof volume.name !== 'string' || + typeof volume.path !== 'string' || + volume.name.trim() === '' || + volume.path.trim() === '' + ) { + return false; + } + }
1-6: File extension: no JSX usage; prefer .tsRename to .ts to avoid unnecessary TSX transforms in tooling.
src-tauri/src/types/docker.rs (2)
6-9: Consider usingu16for port numbers.Port numbers are in the range 0-65535, so
u16is more type-safe thani32, which allows negative values. This prevents invalid port configurations at compile time.Apply this diff:
#[derive(Debug, Clone, Serialize, Deserialize)] pub struct PortMapping { - pub host: i32, - pub container: i32, + pub host: u16, + pub container: u16, }Note: Verify that all consumers of this type (frontend/backend) are updated to use
u16consistently.
29-47: Align port type withPortMappingrecommendation.If you adopt
u16for ports inPortMapping, ensure theportfield here also usesu16for consistency and type safety.Apply this diff if adopting
u16:#[derive(Debug, Clone, Serialize, Deserialize)] pub struct ContainerMetadata { pub id: String, #[serde(rename = "dbType")] pub db_type: String, pub version: String, - pub port: i32, + pub port: u16, pub username: Option<String>, pub password: String, #[serde(rename = "databaseName")] pub database_name: Option<String>, #[serde(rename = "persistData")] pub persist_data: bool, #[serde(rename = "enableAuth")] pub enable_auth: bool, #[serde(rename = "maxConnections")] pub max_connections: Option<i32>, }src/pages/edit-container/hooks/use-database-edit-wizard.ts (2)
14-16: Consider stronger typing forcontainerConfiguration.Using
Record<string, any>bypasses TypeScript's type safety. If the provider-based system allows, consider creating a union type or using generics to provide better type checking and IDE support.For example:
export interface EditDatabaseFormData<T = Record<string, any>> { containerConfiguration: T; }Or create provider-specific form types:
type PostgresConfig = { name: string; port: number; version: string; /* ... */ }; type MongoConfig = { name: string; port: number; version: string; /* ... */ }; type ContainerConfig = PostgresConfig | MongoConfig | /* other types */; export interface EditDatabaseFormData { containerConfiguration: ContainerConfig; }
145-200: Consider replacingalert()with toast notifications.Line 169 uses
alert()for validation errors, which provides a basic but dated user experience. Consider using the toast library (sonner) already in the project for better UX.Example:
import { toast } from 'sonner'; // Replace lines 169-170 with: toast.error('Validation failed', { description: validation.errors.join('\n'), });src-tauri/tests/integration/mongodb_integration_test.rs (1)
15-114: LGTM with optional improvements.The test correctly validates MongoDB container creation with authentication. Two optional improvements:
Hard-coded wait time (Line 99): The 5-second wait is reasonable for MongoDB startup but could be replaced with a retry loop that checks container health.
Port conflicts: Hard-coded port
27018could conflict if tests run in parallel. Consider using a test utility to find available ports or using the--jobs 1flag in test execution.src-tauri/tests/integration/postgresql_integration_test.rs (1)
224-346: Verify port release wait time.Line 294 waits 3 seconds for the old container's port to be released before creating a new container. This may not be sufficient on slower systems or under load. Consider increasing the wait or implementing a retry loop that checks port availability.
Example improvement:
// Instead of fixed wait wait_for_container(3).await; // Use a retry loop for _ in 0..10 { tokio::time::sleep(Duration::from_millis(500)).await; if !container_exists(container_name).await { break; } }src/pages/create-container/steps/review-step.tsx (1)
71-77: Avoid leaking secrets in previewPasswords/tokens in env (-e) are displayed in plain text. Consider masking sensitive keys by default with a “Show secrets” toggle.
src/features/app/use-app.ts (2)
23-27: Add stable deps to useEffectInclude the stable
loadreference to satisfy exhaustive-deps and avoid surprises during hot reloads.- }, [docker.isDockerAvailable]); + }, [docker.isDockerAvailable, containerList.load]);
54-60: Sync after remove (and handle failures)Currently you mutate local state but don’t reconcile with backend. Sync after remove; optionally add try/catch for UX.
- const removeContainer = useCallback( - async (containerId: string) => { - await containerActions.remove(containerId); - containerList.removeLocal(containerId); - }, - [containerActions, containerList], - ); + const removeContainer = useCallback( + async (containerId: string) => { + await containerActions.remove(containerId); + containerList.removeLocal(containerId); + await containerList.sync(); + }, + [containerActions, containerList], + );src/features/databases/types/docker.types.ts (1)
24-28: Make id/password optional to support providers without auth and create flowsSome DBs don’t require auth (e.g., Redis w/o requirepass). Also, an id is typically assigned on create. Relaxing these avoids forced sentinels and improves contract clarity.
-export interface ContainerMetadata { - id: string; +export interface ContainerMetadata { + id?: string; dbType: string; version: string; port: number; username?: string; - password: string; + password?: string; // required only when enableAuth === true databaseName?: string; persistData: boolean; enableAuth: boolean; maxConnections?: number; }If the Rust/Tauri models require these as non-optional, consider making them optional at the request boundary but validated per provider before submission. Please confirm alignment with
src-tauri/src/types/docker.rs. [Based on learnings]Also applies to: 30-41
src/features/databases/providers/mysql.provider.tsx (2)
227-231: Encode credentials in connection stringSpecial chars break the URL. Encode username/password/database.
- getConnectionString(container: Container): string { - const username = container.username || 'root'; - const database = container.databaseName || ''; - return `mysql://${username}:${container.password}@localhost:${container.port}/${database}`; - } + getConnectionString(container: Container): string { + const username = encodeURIComponent(container.username || 'root'); + const password = encodeURIComponent(container.password); + const db = container.databaseName ? `/${encodeURIComponent(container.databaseName)}` : ''; + return `mysql://${username}:${password}@localhost:${container.port}${db}`; + }
188-189: Avoidanyfor configType
configto your form/config contract for safety and DX.src/pages/create-container/steps/database-selection-step.tsx (2)
73-76: Add ARIA roles for keyboard/a11yModel as a radiogroup of radio buttons for proper navigation.
- <motion.div - className="grid grid-cols-2 gap-3" - variants={containerVariants} - > + <motion.div + className="grid grid-cols-2 gap-3" + variants={containerVariants} + role="radiogroup" + aria-label="Database type" + > @@ - <motion.button + <motion.button type="button" className={cn( 'p-4 rounded-lg border-2 transition-all duration-200 ease-out border-white/10 bg-white/10 hover:border-primary hover:shadow-lg hover:opacity-90 group w-full', { 'border-primary bg-primary/20': value === provider.id, }, )} + role="radio" + aria-checked={value === provider.id} + aria-label={provider.name} onClick={() => onChange(provider.id)} disabled={isSubmitting} variants={itemVariants} >Also applies to: 85-96
55-56: Empty providers edge caseIf registry returns an empty array, consider rendering a helpful message.
src/features/databases/providers/mongodb.provider.tsx (3)
26-45: Centralize MongoDB version tags in a shared constants modulePrevents drift and eases maintenance across providers.
141-160: Sharding option is exposed but unusedenableSharding is collected but not applied. Either implement a sharded topology (requires mongos/replica set init scripts) or remove this field to avoid confusion.
Also applies to: 216-219
124-139: authSource field isn’t reflected in connection stringAdvanced field mongoSettings.authSource defaults to "admin" but getConnectionString always uses admin. Either:
- Store the chosen authSource in metadata and use it here, or
- Remove the field if not supported end-to-end.
Also applies to: 248-254
src/features/databases/providers/postgres.provider.tsx (2)
26-69: Centralize PostgreSQL version tags in a shared constants moduleAvoid scatter and ease updates across UI and validation.
217-221: Set shared_preload_libraries via -c, not envPrefer passing server parameters with -c flags. Build command accordingly.
if (config.postgresSettings?.sharedPreloadLibraries) { - envVars.POSTGRES_SHARED_PRELOAD_LIBRARIES = - config.postgresSettings.sharedPreloadLibraries; + // Use -c to set server parameter at startup + command.push( + '-c', + `shared_preload_libraries=${config.postgresSettings.sharedPreloadLibraries}` + ); } - return { + return { image: `postgres:${config.version}`, envVars, ports: [{ host: config.port, container: this.containerPort }], volumes: config.persistData ? [{ name: `${config.name}-data`, path: this.dataPath }] : [], - command: [], + command, };Also applies to: 222-230
src-tauri/tests/integration/mysql_integration_test.rs (2)
38-40: Consider dynamic port allocation to avoid conflicts.Hard-coded ports (3307, 3308) can cause test failures if the ports are already in use or when running tests in parallel. Consider using dynamic port allocation or making ports configurable via environment variables.
Example approach using a port allocation helper:
// Add to utils.rs fn get_available_port() -> u16 { use std::net::TcpListener; let listener = TcpListener::bind("127.0.0.1:0").unwrap(); listener.local_addr().unwrap().port() }Then use it in tests:
+ let host_port = get_available_port(); ports: vec![PortMapping { - host: 3307, + host: host_port, container: 3306, }],Also applies to: 131-133
1-191: Optional: Consider adding actual MySQL connectivity tests.These tests validate Docker command generation and container lifecycle but don't verify that MySQL is actually accessible and functioning. While this may be intentional for integration tests focused on Docker operations, adding a simple connection attempt would increase confidence that the container is fully operational.
Example addition after container creation:
// Optional: Verify MySQL is actually accessible // Requires adding mysql client crate as dev-dependency use mysql::*; let opts = OptsBuilder::new() .ip_or_hostname(Some("127.0.0.1")) .tcp_port(3307) .user(Some("testuser")) .pass(Some("testpass123")) .db_name(Some("testdb")); // Simple connection test with retries for attempt in 1..=5 { match Conn::new(opts.clone()) { Ok(_) => { println!("✅ MySQL connection successful"); break; } Err(e) if attempt == 5 => panic!("Failed to connect to MySQL after 5 attempts: {}", e), Err(_) => tokio::time::sleep(tokio::time::Duration::from_secs(2)).await, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (67)
.github/copilot-instructions.md(0 hunks)package.json(1 hunks)src-tauri/src/commands/app.rs(0 hunks)src-tauri/src/commands/database.rs(4 hunks)src-tauri/src/lib.rs(1 hunks)src-tauri/src/services/docker.rs(1 hunks)src-tauri/src/types/docker.rs(1 hunks)src-tauri/src/types/mod.rs(1 hunks)src-tauri/src/types/requests.rs(0 hunks)src-tauri/src/types/settings.rs(0 hunks)src-tauri/tests/integration/container_update_test.rs(0 hunks)src-tauri/tests/integration/mongodb_integration_test.rs(1 hunks)src-tauri/tests/integration/mysql_integration_test.rs(1 hunks)src-tauri/tests/integration/postgresql_integration_test.rs(6 hunks)src-tauri/tests/integration/redis_integration_test.rs(2 hunks)src-tauri/tests/integration/utils.rs(2 hunks)src-tauri/tests/integration_tests.rs(1 hunks)src-tauri/tests/unit/docker_service_test.rs(1 hunks)src-tauri/tests/unit/generic_commands_test.rs(1 hunks)src-tauri/tests/unit/services/docker_command_builder_test.rs(0 hunks)src-tauri/tests/unit/services/docker_service_test.rs(0 hunks)src-tauri/tests/unit/services/volume_migration_test.rs(0 hunks)src-tauri/tests/unit_services.rs(0 hunks)src-tauri/tests/unit_tests.rs(1 hunks)src/features/app/api/__tests__/app.api.test.ts(0 hunks)src/features/app/use-app.ts(3 hunks)src/features/containers/api/__tests__/containers.api.test.ts(0 hunks)src/features/containers/api/containers.api.ts(0 hunks)src/features/containers/hooks/use-container-actions.ts(0 hunks)src/features/containers/services/__tests__/container.service.test.ts(0 hunks)src/features/containers/services/container.service.ts(0 hunks)src/features/databases/api/databases.api.ts(1 hunks)src/features/databases/components/dynamic-form-field.tsx(1 hunks)src/features/databases/components/dynamic-form-section.tsx(1 hunks)src/features/databases/hooks/use-database-actions.ts(1 hunks)src/features/databases/hooks/use-database-list.ts(2 hunks)src/features/databases/hooks/use-registry.ts(1 hunks)src/features/databases/providers/mongodb.provider.tsx(1 hunks)src/features/databases/providers/mysql.provider.tsx(1 hunks)src/features/databases/providers/postgres.provider.tsx(1 hunks)src/features/databases/providers/redis.provider.tsx(1 hunks)src/features/databases/registry/database-provider.interface.ts(1 hunks)src/features/databases/registry/database-registry.ts(1 hunks)src/features/databases/types/docker.types.ts(1 hunks)src/features/databases/types/form.types.ts(1 hunks)src/features/docker/hooks/use-docker-status.ts(2 hunks)src/pages/create-container/components/DatabaseSelectionForm.tsx(1 hunks)src/pages/create-container/hooks/use-container-creation-wizard.ts(4 hunks)src/pages/create-container/schemas/database-form.schema.ts(0 hunks)src/pages/create-container/steps/container-configuration-step.tsx(3 hunks)src/pages/create-container/steps/database-selection-step.tsx(5 hunks)src/pages/create-container/steps/review-step.tsx(2 hunks)src/pages/edit-container/components/EditContainerForm.tsx(2 hunks)src/pages/edit-container/hooks/use-container-edit.ts(0 hunks)src/pages/edit-container/hooks/use-database-edit-wizard.ts(1 hunks)src/pages/main/MainPage.tsx(0 hunks)src/pages/main/hooks/use-main-page.ts(0 hunks)src/shared/components/DatabaseConfigPanel.tsx(0 hunks)src/shared/components/ui/checkbox.tsx(1 hunks)src/shared/types/container.ts(1 hunks)src/shared/types/settings.ts(0 hunks)src/shared/utils/container.ts(0 hunks)src/shared/utils/docker.ts(0 hunks)src/test/unit/database-registry.test.ts(1 hunks)src/test/unit/providers.test.ts(1 hunks)src/test/utils/mock-providers.ts(1 hunks)src/test/utils/test-utils.tsx(1 hunks)
💤 Files with no reviewable changes (23)
- src/features/app/api/tests/app.api.test.ts
- src-tauri/tests/integration/container_update_test.rs
- .github/copilot-instructions.md
- src-tauri/src/types/requests.rs
- src/pages/main/MainPage.tsx
- src/shared/types/settings.ts
- src-tauri/src/commands/app.rs
- src/pages/main/hooks/use-main-page.ts
- src/features/containers/hooks/use-container-actions.ts
- src/features/containers/api/tests/containers.api.test.ts
- src-tauri/tests/unit/services/docker_command_builder_test.rs
- src/pages/edit-container/hooks/use-container-edit.ts
- src/shared/utils/docker.ts
- src/features/containers/api/containers.api.ts
- src-tauri/tests/unit_services.rs
- src/shared/components/DatabaseConfigPanel.tsx
- src/features/containers/services/container.service.ts
- src/features/containers/services/tests/container.service.test.ts
- src-tauri/tests/unit/services/docker_service_test.rs
- src-tauri/src/types/settings.rs
- src-tauri/tests/unit/services/volume_migration_test.rs
- src/shared/utils/container.ts
- src/pages/create-container/schemas/database-form.schema.ts
🔇 Additional comments (33)
package.json (1)
39-39: LGTM! Dependency addition aligns with new UI component.The
@radix-ui/react-checkboxdependency is correctly added to support the new Checkbox component. Version^1.3.3is appropriate and consistent with other Radix UI dependencies in the project.Based on learnings.
src/shared/components/ui/checkbox.tsx (2)
1-4: LGTM! Clean and appropriate imports.All imports are necessary and properly organized. The component correctly leverages Radix UI primitives alongside Lucide icons and the internal
cnutility.
6-27: Well-implemented Radix-based checkbox component.The component correctly follows Radix UI patterns:
- Proper typing with
React.ComponentProps<typeof CheckboxPrimitive.Root>- Ref forwarding works automatically through the Radix primitive (no explicit
forwardRefneeded)- Comprehensive state-based styling covering checked, focus, disabled, and validation states
- Accessible by default via Radix's built-in ARIA patterns
The
data-slotattributes enable consistent querying and styling across the design system.Based on learnings.
src-tauri/tests/unit/docker_service_test.rs (1)
158-174: LGTM!The serialization/deserialization test provides valuable coverage of the data contract and ensures the types can round-trip through JSON.
src/features/docker/hooks/use-docker-status.ts (2)
28-35: LGTM!The simplified logic using direct status comparison (
status.status !== 'running') is clearer and removes the dependency on a helper function.
119-119: LGTM!The direct status check is consistent with the change on line 28 and improves code clarity.
src/test/unit/database-registry.test.ts (1)
1-109: LGTM!Comprehensive test coverage for the DatabaseRegistry, including factory pattern verification, retrieval methods, existence checks, counting, and validation of all real providers (PostgreSQL, MySQL, Redis, MongoDB).
src/features/databases/registry/database-registry.ts (1)
56-65: LGTM!The factory pattern with a singleton instance provides a clean API. The centralized list of providers (PostgreSQL, MySQL, Redis, MongoDB) makes it easy to add new providers by extending this array.
src-tauri/tests/integration/utils.rs (3)
76-101: LGTM!The
get_container_portfunction follows the existing pattern for querying Docker container information and handles errors appropriately.
144-151: LGTM!The
volume_existsfunction provides a simple, boolean check consistent with other existence verification functions in the file.
153-165: LGTM!The
run_docker_commandfunction provides good error handling with descriptive error messages and follows Rust best practices for Result types.src/features/databases/hooks/use-database-actions.ts (1)
15-46: LGTM!The
start,stop, andremoveactions follow a consistent pattern with appropriate error handling and user feedback via toasts.src/features/databases/hooks/use-database-list.ts (1)
1-103: LGTM!Clean refactoring that updates the hook name and API source to align with database-centric terminology. The use of absolute imports (
@/) and the switch fromcontainersApitodatabasesApiimproves consistency across the codebase.src-tauri/tests/unit/generic_commands_test.rs (1)
1-41: Missinguuid::Uuidimport breaks tests
create_test_docker_requestcallsuuid::Uuid::new_v4()but the module never importsuuid. The test module only bringsdocker_*types andHashMapinto scope viause super::*, so this won’t compile. Adduse uuid::Uuid;inside the test module (or at top-level) and useUuid::new_v4().#[cfg(test)] mod generic_commands_tests { use super::*; + use uuid::Uuid; @@ - id: uuid::Uuid::new_v4().to_string(), + id: Uuid::new_v4().to_string(),Likely an incorrect or invalid review comment.
src-tauri/src/types/docker.rs (3)
11-16: LGTM!The
VolumeMountstruct is well-defined for its purpose as a data contract.
18-27: LGTM!The
DockerRunArgsstruct provides a clean, database-agnostic abstraction for Docker run parameters. The use of#[serde(rename = "envVars")]maintains consistency with frontend naming conventions.
49-56: LGTM!The
DockerRunRequeststruct effectively encapsulates all necessary information for creating/updating containers in a single payload.src/pages/edit-container/hooks/use-database-edit-wizard.ts (2)
27-33: LGTM!The form setup correctly defers validation to the provider-based system in the
savefunction, which is appropriate given the dynamic configuration needs across different database types.
40-79: LGTM!The
loadContainercallback correctly handles loading, error cases, and form population. The provider lookup ensures compatibility with the registry system.src-tauri/tests/integration/mongodb_integration_test.rs (2)
116-213: LGTM!The test correctly validates MongoDB container creation with volume persistence. The volume cleanup is properly handled in both success and error paths.
215-293: LGTM!The test correctly validates MongoDB container creation without authentication, including proper assertions that auth environment variables are not present in the generated command.
src-tauri/tests/integration/postgresql_integration_test.rs (2)
15-123: LGTM!The test correctly validates PostgreSQL container creation with proper assertions for image, ports, environment variables, and container status. The cleanup is handled appropriately.
125-222: LGTM!The test correctly validates PostgreSQL container creation with volume persistence, including proper cleanup of both container and volume resources.
src-tauri/tests/integration/redis_integration_test.rs (3)
15-98: LGTM!The test correctly validates Redis container creation without authentication, including proper verification of the generated command and container status.
100-179: LGTM!The test correctly validates Redis container creation with password authentication using the
--requirepasscommand-line option, which is the standard approach for Redis authentication.
181-278: LGTM!The test correctly validates Redis container creation with AOF (Append-Only File) persistence enabled, including proper volume management and cleanup.
src/pages/create-container/steps/review-step.tsx (1)
61-63: Container name validation/sanitizationEnsure upstream validation enforces Docker’s name rules (alphanum, '.', '_', '-', no spaces). Otherwise preview shows a command that will fail at runtime.
src/features/databases/providers/redis.provider.tsx (2)
117-123: Optional password + min length: ensure validator doesn’t reject emptyField is not required but has min: 4. Confirm the form only enforces min when a value is present; otherwise users can’t leave it empty.
245-248: Connection string looks correctFormat redis://[:password@]localhost:port is correct.
src/features/databases/providers/postgres.provider.tsx (2)
234-238: Connection string looks correctpostgresql://user:pass@localhost:port/dbname is fine.
208-211: No action needed for POSTGRES_HOST_AUTH_METHOD
This variable accepts any valid pg_hba.conf method (e.g., trust, md5, scram-sha-256, password, peer, ident, cert, pam, ldap, radius, gss, sspi, reject) and on container init appendshost all all all <method>topg_hba.conf. Omitting it defaults to md5.src-tauri/tests/integration/mysql_integration_test.rs (2)
13-23: Well-structured test setup with proper cleanup.The Docker availability check and initial cleanup pattern is consistent with best practices for integration tests. Early returns when Docker is unavailable prevent false negatives in CI environments without Docker.
80-85: Good error handling with cleanup on failure.The error handling properly cleans up resources before panicking, preventing resource leaks when container creation fails.
| // Create database object using metadata | ||
| let database = DatabaseContainer { | ||
| id: container_id.clone(), | ||
| id: request.metadata.id.clone(), | ||
| name: request.name.clone(), | ||
| db_type: request.db_type, | ||
| version: request.version, | ||
| db_type: request.metadata.db_type, | ||
| version: request.metadata.version, | ||
| status: "running".to_string(), | ||
| port: request.port, | ||
| port: request.metadata.port, | ||
| created_at: chrono::Utc::now().format("%Y-%m-%d").to_string(), | ||
| max_connections: request.max_connections.unwrap_or(100), | ||
| max_connections: request.metadata.max_connections.unwrap_or(100), | ||
| container_id: Some(real_container_id.clone()), | ||
| stored_password: Some(request.password.clone()), | ||
| stored_username: request.username.clone(), | ||
| stored_database_name: request.database_name.clone(), | ||
| stored_persist_data: request.persist_data, | ||
| stored_enable_auth: request.enable_auth, | ||
| stored_password: Some(request.metadata.password.clone()), | ||
| stored_username: request.metadata.username.clone(), | ||
| stored_database_name: request.metadata.database_name.clone(), | ||
| stored_persist_data: request.metadata.persist_data, | ||
| stored_enable_auth: request.metadata.enable_auth, | ||
| }; |
There was a problem hiding this comment.
Avoid persisting plaintext passwords
stored_password is saved in-memory and to disk. Encrypt with OS keyring/tauri secret storage, or store a reference/token. At minimum, mask in logs/exports.
🤖 Prompt for AI Agents
In src-tauri/src/commands/database.rs around lines 84 to 100, the code persists
plaintext passwords via stored_password; instead, stop saving plaintext by
removing or making stored_password transient in the persisted DatabaseContainer
and save the secret via the OS/tauri secure storage or keyring (store an opaque
token/reference or encrypted blob), update the creation flow to write the actual
password into the secure store and persist only the secure reference/token in
the container, ensure any logs/exports mask or omit the password, and add any
necessary struct/migration changes and error handling for secure-store
operations.
…istry for provider access
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/create-container/steps/database-selection-step.tsx (1)
55-55: Consider adding empty state handling.If the registry returns an empty array, users will see a blank screen with no feedback. Consider adding a fallback message for better UX.
Example implementation after line 55:
const providers = databaseRegistry.getAll(); if (providers.length === 0) { return ( <div className="text-center text-muted-foreground py-8"> No database providers available </div> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pages/create-container/steps/container-configuration-step.tsx(3 hunks)src/pages/create-container/steps/database-selection-step.tsx(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/pages/create-container/steps/container-configuration-step.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/create-container/steps/database-selection-step.tsx (2)
src/pages/create-container/hooks/use-container-creation-wizard.ts (1)
CreateDatabaseFormData(14-19)src/features/databases/registry/database-registry.ts (1)
databaseRegistry(65-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend (Tests + Build)
🔇 Additional comments (2)
src/pages/create-container/steps/database-selection-step.tsx (2)
3-3: Excellent refactor to registry-based providers!The shift from hard-coded database options to the dynamic
databaseRegistrypattern improves maintainability and makes it easier to add new database providers without modifying this component.Also applies to: 5-5, 8-8
76-121: Provider-based rendering looks solid.The rendering logic correctly adapts to the registry pattern, using
provider.idfor keys and form values, and properly accessing provider properties for styling and display.
…lumes after migration
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src-tauri/src/commands/database.rs (3)
95-99: Avoid persisting plaintext passwords.The code stores plaintext credentials (
stored_password,stored_username) in-memory and persists them to disk. Use OS keyring/Tauri secret storage, or at minimum, encrypt before persisting.Based on learnings.
293-293: Don't overwrite stored_password with an empty/unchanged value.This line unconditionally assigns
request.metadata.passwordtocontainer.stored_password, which can overwrite an existing password with an empty string.Based on learnings.
320-331: Partial improvement: deferred volume cleanup reduces data-loss risk, but rollback still missing.Deferring old volume removal until after store save (lines 326-331) is an improvement from the previous version. However, if the store save fails (lines 321-322), the new container and volumes remain running without a rollback, leaving an inconsistent state.
Consider capturing the new container ID and volumes created, then on store save failure:
- Stop and remove the new container
- Remove the new volumes
- Optionally restart the old container if it was stopped
Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src-tauri/src/commands/database.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src-tauri/src/commands/database.rs (2)
src/features/databases/types/docker.types.ts (1)
DockerRunRequest(24-28)src-tauri/src/services/docker.rs (1)
new(13-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend (Tests + Build)
🔇 Additional comments (4)
src-tauri/src/commands/database.rs (4)
28-82: Well-structured error handling with cleanup.The error handling comprehensively covers port conflicts, name conflicts, and generic Docker errors, with proper cleanup of containers and volumes on failure.
114-132: Good rollback on store save failure.The cleanup logic properly removes the container and volumes from Docker when the store save fails, preventing orphaned resources.
158-166: Good recreation detection logic.Capturing
previous_nameand determining recreation needs based on name/port/persistence changes is well-structured.
183-221: Well-defined volume migration scenarios.The four migration cases (name change with data, enabling persistence, disabling persistence, name change without data) are clearly separated and correctly handled.
…h creation behavior
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src-tauri/src/commands/database.rs (3)
95-95: Plaintext password storage remains unaddressed.The plaintext password is still stored in
stored_passwordat line 95 (and line 293 in the update flow). This was previously flagged but remains unresolved.As noted in the past review comment on lines 84-100, passwords should be encrypted using OS keyring/Tauri secret storage rather than persisted in plaintext.
293-293: Unconditional password overwrite can lose existing credentials.This line unconditionally overwrites
stored_password, even if the request contains an empty or placeholder password. If the frontend sends an unchanged/empty password during an update, the stored password is lost.This issue was previously flagged in past review comments (lines 293-296).
Apply this fix to preserve existing passwords when none is provided:
- container.stored_password = Some(request.metadata.password); + if !request.metadata.password.is_empty() { + container.stored_password = Some(request.metadata.password.clone()); + }Alternatively, if
passwordshould be optional in metadata, changeContainerMetadata.passwordtoOption<String>and only update whenSome(non_empty)is provided.
183-196: Volume migration occurs before container recreation succeeds.When a name change triggers migration (lines 183-196), the data is moved from the old volume to the new volume immediately. If the subsequent
run_containercall fails (line 228), the error handler cleans up the new volume (lines 236-241) but doesn't restore the old volume, resulting in data loss.Consider one of these approaches:
Option 1 (Safer): Defer migration until after container success:
- Create the new volume but leave it empty
- Run the container with the new volume
- Only after success, perform the migration in the background or on next access
- Then delete the old volume
Option 2: Add migration rollback to the error handler:
Err(error) => { // Cleanup resources on error let _ = docker_service .force_remove_container_by_name(&app, &request.name) .await; + // If migration occurred, attempt to restore old volume + if name_changed && container.stored_persist_data && request.metadata.persist_data { + let old_volume_name = format!("{}-data", previous_name); + let new_volume_name = format!("{}-data", request.name); + if let Some(vol) = new_volumes.first() { + let _ = docker_service + .migrate_volume_data(&app, &new_volume_name, &old_volume_name, &vol.path) + .await; + } + } + // Cleanup new volumes if they were created for volume in new_volumes {
🧹 Nitpick comments (1)
src-tauri/src/commands/database.rs (1)
44-80: Consider extracting duplicate error handling logic.The error handling for port conflicts, name conflicts, and generic Docker errors is duplicated between
create_container_from_docker_args(lines 44-80) andupdate_container_from_docker_args(lines 244-281).Extract this into a helper function to reduce duplication:
fn map_docker_error(error: String, port: u16, name: &str) -> String { if error.contains("port is already allocated") || error.contains("Bind for") { let port_error = CreateContainerError { error_type: "PORT_IN_USE".to_string(), message: format!("Port {} is already in use", port), port: Some(port), details: Some("You can change the port in the configuration and try again.".to_string()), }; return serde_json::to_string(&port_error) .unwrap_or_else(|_| "Port in use error".to_string()); } if error.contains("name is already in use") || error.contains("already exists") { let name_error = CreateContainerError { error_type: "NAME_IN_USE".to_string(), message: format!("A container with the name '{}' already exists", name), port: None, details: Some("Change the container name and try again.".to_string()), }; return serde_json::to_string(&name_error) .unwrap_or_else(|_| "Name in use error".to_string()); } let generic_error = CreateContainerError { error_type: "DOCKER_ERROR".to_string(), message: "Error creating/updating container".to_string(), port: None, details: Some(error.to_string()), }; serde_json::to_string(&generic_error) .unwrap_or_else(|_| format!("Docker command failed: {}", error)) }Then replace both error handling blocks with:
return Err(map_docker_error(error, request.metadata.port, &request.name));Also applies to: 244-281
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src-tauri/src/commands/database.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src-tauri/src/commands/database.rs (2)
src/features/databases/types/docker.types.ts (1)
DockerRunRequest(24-28)src-tauri/src/services/docker.rs (1)
new(13-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend (Tests + Build)
🔇 Additional comments (4)
src-tauri/src/commands/database.rs (4)
353-397: LGTM: Database synchronization logic is sound.The function correctly loads from store, syncs container status with Docker, updates in-memory state, and persists changes. The error propagation is appropriate.
399-440: LGTM: Container lifecycle commands are implemented correctly.The
start_container,stop_container, andremove_containercommands follow consistent patterns:
- Retrieve container info from state
- Execute Docker operation
- Update in-memory state
- Persist to store
The
remove_containercommand correctly cleans up persistent volumes when needed (lines 510-518).Also applies to: 442-483, 485-532
320-340: Good: Rollback on store save failure.The update flow now properly rolls back on store save failure (lines 320-340), cleaning up the new container and volumes if the container was recreated. This addresses the past review comment about aligning create/update behaviors.
This is a significant improvement in error handling consistency.
342-348: Good: Old volume cleanup deferred until after store save.The old volume cleanup now correctly occurs after the store save succeeds (lines 342-348), which addresses the past review comment about data-loss risk. This ensures the old volume is only removed once we're confident the update completed successfully.
…reSQL, and Redis integration tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src-tauri/tests/integration/utils.rs (1)
115-131: Consider minor optimization: avoid delay after final attempt.The current logic delays between attempts even after the final unsuccessful check. While not critical, you could skip the delay after
max_attemptsis reached:for attempt in 1..=max_attempts { if container_exists(container_name).await { if let Some(status) = get_container_status(container_name).await { if status.contains("Up") { println!( "✅ Container '{}' is ready (attempt {}/{})", container_name, attempt, max_attempts ); return true; } } } if attempt < max_attempts { tokio::time::sleep(tokio::time::Duration::from_secs(delay_secs)).await; } }Note: This optimization is already implemented correctly (line 128 checks
attempt < max_attempts).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src-tauri/tests/integration/mongodb_integration_test.rs(1 hunks)src-tauri/tests/integration/mysql_integration_test.rs(1 hunks)src-tauri/tests/integration/postgresql_integration_test.rs(5 hunks)src-tauri/tests/integration/redis_integration_test.rs(2 hunks)src-tauri/tests/integration/utils.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src-tauri/tests/integration/mysql_integration_test.rs
- src-tauri/tests/integration/redis_integration_test.rs
🧰 Additional context used
🧬 Code graph analysis (2)
src-tauri/tests/integration/postgresql_integration_test.rs (2)
src/features/databases/types/docker.types.ts (5)
ContainerMetadata(30-41)DockerRunArgs(16-22)DockerRunRequest(24-28)PortMapping(6-9)VolumeMount(11-14)src-tauri/tests/integration/utils.rs (10)
run_docker_command(177-188)clean_container(18-28)wait_for_container_ready(105-138)container_exists(31-47)get_container_status(50-74)create_volume(141-153)clean_volume(156-165)volume_exists(168-174)docker_available(9-15)get_container_port(77-101)
src-tauri/tests/integration/mongodb_integration_test.rs (2)
src/features/databases/types/docker.types.ts (5)
ContainerMetadata(30-41)DockerRunArgs(16-22)DockerRunRequest(24-28)PortMapping(6-9)VolumeMount(11-14)src-tauri/tests/integration/utils.rs (9)
docker_available(9-15)clean_container(18-28)run_docker_command(177-188)wait_for_container_ready(105-138)container_exists(31-47)get_container_status(50-74)clean_volume(156-165)create_volume(141-153)volume_exists(168-174)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend (Tests + Build)
🔇 Additional comments (9)
src-tauri/tests/integration/utils.rs (3)
76-101: LGTM!The function correctly retrieves container port mappings using Docker CLI, with appropriate error handling and a consistent pattern matching other utility functions in this module.
103-138: LGTM!The readiness check logic is well-implemented with proper retry mechanism, logging, and efficient delay handling (only delays between attempts, not after the final one).
167-174: LGTM!Clean implementation that correctly checks volume existence using Docker CLI.
src-tauri/tests/integration/postgresql_integration_test.rs (3)
15-126: LGTM!Well-structured integration test with proper setup, execution, validation, and cleanup. The use of
wait_for_container_readyensures the container is fully operational before verification.
128-229: LGTM!Comprehensive volume integration test with proper lifecycle management. The warning-only approach to volume creation failures (lines 193-195) is appropriate since Docker auto-creates volumes when needed.
231-363: LGTM with observation on timing.The port update test is well-structured. The 2-second delay (lines 306-307) after container removal helps ensure port release, though on heavily loaded systems this might occasionally be insufficient. This is an acceptable tradeoff in integration tests.
Consider whether the fixed 2-second delay is sufficient for your CI/CD environment. If tests become flaky, you might need to increase this or implement a retry mechanism for the port binding step.
src-tauri/tests/integration/mongodb_integration_test.rs (3)
15-118: LGTM!MongoDB integration test is well-structured with appropriate authentication setup. The 15-attempt readiness check (line 101) provides sufficient time for MongoDB initialization.
120-221: LGTM!Volume integration test correctly mounts to MongoDB's data directory (
/data/db) with proper lifecycle management and cleanup.
223-305: LGTM!The no-auth test properly validates that authentication environment variables are absent from the generated command. The negative assertions (lines 269-276) are essential for confirming the configuration.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src-tauri/src/commands/database.rs (1)
84-100: Avoid persisting plaintext passwords
stored_password(line 95) is saved in-memory and to disk. Encrypt with OS keyring/tauri secret storage, or store a reference/token. At minimum, mask in logs/exports.Based on past review comments.
🧹 Nitpick comments (1)
src-tauri/src/commands/database.rs (1)
44-80: Consider extracting duplicate error handlingThe error handling logic for port conflicts, name conflicts, and generic Docker errors is duplicated between the create and update functions. Extracting this into a shared helper function would improve maintainability and reduce code duplication.
Example helper function:
fn handle_docker_run_error(error: String, request_name: &str, request_port: u16) -> String { if error.contains("port is already allocated") || error.contains("Bind for") { let port_error = CreateContainerError { error_type: "PORT_IN_USE".to_string(), message: format!("Port {} is already in use", request_port), port: Some(request_port), details: Some("You can change the port in the configuration and try again.".to_string()), }; return serde_json::to_string(&port_error) .unwrap_or_else(|_| "Port in use error".to_string()); } if error.contains("name is already in use") || error.contains("already exists") { let name_error = CreateContainerError { error_type: "NAME_IN_USE".to_string(), message: format!("A container with the name '{}' already exists", request_name), port: None, details: Some("Change the container name and try again.".to_string()), }; return serde_json::to_string(&name_error) .unwrap_or_else(|_| "Name in use error".to_string()); } let generic_error = CreateContainerError { error_type: "DOCKER_ERROR".to_string(), message: "Error with container operation".to_string(), port: None, details: Some(error.to_string()), }; serde_json::to_string(&generic_error) .unwrap_or_else(|_| format!("Docker command failed: {}", error)) }Also applies to: 243-281
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src-tauri/src/commands/database.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src-tauri/src/commands/database.rs (2)
src/features/databases/types/docker.types.ts (1)
DockerRunRequest(24-28)src-tauri/src/services/docker.rs (1)
new(13-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend (Tests + Build)
🔇 Additional comments (4)
src-tauri/src/commands/database.rs (4)
294-297: Password handling correctly improvedThe code now checks for non-empty passwords before updating
stored_password, addressing the previous concern about overwriting with empty values.
325-345: Store save rollback implemented correctlyThe rollback logic now properly cleans up Docker resources (container and volumes) when store save fails, addressing the previous concern about inconsistent state.
347-353: Old volume cleanup correctly deferredThe old volume is now removed only after successful store save, eliminating the data-loss risk identified in previous reviews.
358-537: LGTM on utility commandsThe
get_all_databases,start_container,stop_container, andremove_containerfunctions follow consistent patterns for state management and Docker interactions. The logic and error handling appear correct.
…nstead of environment variables
…ed command flag handling and documentation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/features/databases/providers/redis.provider.tsx (3)
26-63: Consider extracting version list to configuration.The hardcoded version list makes updates tedious when new Redis versions are released. Consider extracting to a shared configuration file or constants module for easier maintenance.
Apply this approach:
Create
src/features/databases/config/versions.ts:export const REDIS_VERSIONS = [ // Redis 8.2 '8.2.2', '8.2', // ... rest of versions ];Then import and use:
+import { REDIS_VERSIONS } from '../config/versions'; + export class RedisDatabaseProvider implements DatabaseProvider { // ... - readonly versions = [ - // Redis 8.2 - '8.2.2', - '8.2', - // ... - ]; + readonly versions = REDIS_VERSIONS;Based on past review comment from Copilot.
127-198: Consider adding validation for advanced configuration fields.While the field definitions are clear, consider adding validation to prevent invalid configurations:
maxMemory: Validate format matches pattern like\d+(mb|gb|kb|b)(e.g., "256mb", "1gb")save: Validate format contains pairs of numbers (even token count, all numeric)maxClients: Add min/max range validation (e.g., min: 1, max: 100000)Example validation for the save field:
{ name: 'redisSettings.save', label: 'RDB Snapshots', type: 'text', placeholder: '900 1 300 10 60 10000', + validation: { + pattern: /^(\d+\s+\d+\s*)+$/, + message: 'Must be pairs of numbers (seconds changes) separated by spaces', + }, helpText: '...', },
263-278: Validation covers the essentials.The
validateConfigmethod provides basic but adequate validation for critical fields. For enhanced user experience, consider expanding validation to include:
- maxMemory format checking (e.g., valid suffixes like mb, gb)
- RDB save format verification (pairs of numbers)
- Port range validation (1024-65535)
- maxClients range validation
However, the current implementation is acceptable for the initial release.
src/features/databases/providers/mysql.provider.tsx (1)
59-62: Consider validating container name format.While the minimum length check is good, Docker container names must follow a specific pattern (alphanumeric characters, hyphens, and underscores; must start with alphanumeric). Consider adding format validation to prevent runtime errors.
Example enhancement:
validation: { min: 3, - message: 'Container name must be at least 3 characters', + pattern: /^[a-zA-Z0-9][a-zA-Z0-9_-]*$/, + message: 'Container name must be at least 3 characters and contain only letters, numbers, hyphens, and underscores', },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/features/databases/providers/mysql.provider.tsx(1 hunks)src/features/databases/providers/redis.provider.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/features/databases/providers/mysql.provider.tsx (4)
src/features/databases/registry/database-provider.interface.ts (2)
DatabaseProvider(14-73)FieldsOptions(6-8)src/features/databases/types/form.types.ts (1)
FieldGroup(37-41)src/features/databases/types/docker.types.ts (2)
DockerRunArgs(16-22)ValidationResult(43-46)src/shared/types/container.ts (1)
Container(14-29)
src/features/databases/providers/redis.provider.tsx (4)
src/features/databases/registry/database-provider.interface.ts (2)
DatabaseProvider(14-73)FieldsOptions(6-8)src/features/databases/types/form.types.ts (1)
FieldGroup(37-41)src/features/databases/types/docker.types.ts (2)
DockerRunArgs(16-22)ValidationResult(43-46)src/shared/types/container.ts (1)
Container(14-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend (Tests + Build)
🔇 Additional comments (6)
src/features/databases/providers/redis.provider.tsx (4)
66-125: LGTM! Field definitions are well-structured.The basic and authentication fields are properly defined with appropriate validation rules, helpful text, and correct handling of edit mode for immutable fields.
224-239: LGTM! RDB save argument parsing is correct.The critical issue flagged in the previous review has been successfully resolved. The code now correctly:
- Splits the save string into whitespace-separated tokens
- Iterates through pairs (seconds, changes)
- Pushes each pair as separate arguments:
--save <seconds> <changes>- Safely handles odd token counts by checking both values exist
This produces the correct Redis command format:
redis-server --save 900 1 --save 300 10 ...
201-255: LGTM! Docker argument building is well-structured.The
buildDockerArgsmethod correctly:
- Builds Redis command arguments for all configuration options
- Uses proper Redis CLI flags (--requirepass, --maxmemory, --appendonly, etc.)
- Conditionally includes volumes based on persistence settings
- Returns a well-formed DockerRunArgs object
258-286: LGTM! Utility methods are correctly implemented.The utility methods properly handle Redis-specific requirements:
- Connection string uses correct
redis://protocol with optional auth- Username is undefined (Redis doesn't use usernames)
- Authentication is marked as optional (correct for Redis)
src/features/databases/providers/mysql.provider.tsx (2)
185-200: Excellent fix: correctly using mysqld command flags.The implementation now properly uses
--character-set-server,--collation-server, and--sql-modeflags instead of environment variables. This is the correct approach for the official MySQL image.
223-224: Sync validation logic with field requirements.When the field validation is updated to require a minimum of 8 characters (as suggested above), ensure this validation logic is updated accordingly to maintain consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pages/edit-container/hooks/use-database-edit-wizard.ts (1)
112-114: Complete the validation by adding checks for name and version.The past review comment suggested validating port, name, and version before transformation. Currently, only
portis validated. Lines 128 and 133 usecontainerConfiguration.nameandcontainerConfiguration.versionwithout validation, which could result in undefined values being passed to the Docker request.Apply this diff to add the missing validations:
// Validate required fields if (!containerConfiguration.port) { throw new Error('Port is required'); } + if (!containerConfiguration.name) { + throw new Error('Container name is required'); + } + if (!containerConfiguration.version) { + throw new Error('Version is required'); + }
🧹 Nitpick comments (2)
src/pages/edit-container/hooks/use-database-edit-wizard.ts (2)
174-174: Consider replacingalert()with a more user-friendly notification.Using
alert()provides a blocking browser dialog which may not align with modern UX expectations. Consider using a toast notification library or an inline error display mechanism for a smoother user experience.
72-72: Consider cleaning up debug logging statements.Multiple
console.log,console.error, andconsole.warnstatements are present throughout the hook (lines 72, 153, 173, 181, 191, 198). For production code, consider either removing debug logs or migrating to a structured logging solution for better observability.Also applies to: 153-153, 173-173, 181-181, 191-191, 198-198
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/edit-container/hooks/use-database-edit-wizard.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/edit-container/hooks/use-database-edit-wizard.ts (4)
src/shared/types/container.ts (1)
Container(14-29)src/features/databases/api/databases.api.ts (1)
databasesApi(29-104)src/features/databases/registry/database-registry.ts (1)
databaseRegistry(65-65)src/features/databases/types/docker.types.ts (1)
DockerRunRequest(24-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend (Tests + Build)
… MongoDB storage engine requirements
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/features/databases/providers/mongodb.provider.tsx (1)
221-231: Consider additional guardrails for inMemory storage engine.While the console warning (line 228) alerts developers, the inMemory storage engine will still fail silently at container startup when using Community images. Users may not notice the warning in browser console logs and will see cryptic Docker errors instead.
Consider one of these approaches:
- Remove inMemory from the options list (lines 169) and only support wiredTiger for Community images
- Add runtime validation in
validateConfigto check if the selected image/version supports inMemory before allowing it- Add a more prominent UI warning (e.g., an alert banner) when inMemory is selected, explaining it requires Enterprise Edition
Example validation approach:
validateConfig(config: any): ValidationResult { const errors: string[] = []; if (!config.password || config.password.length < 4) { errors.push('Password must be at least 4 characters'); } if (!config.version) { errors.push('MongoDB version is required'); } if (!config.username) { errors.push('Username is required'); } + if (config.mongoSettings?.storageEngine === 'inMemory') { + errors.push( + 'inMemory storage engine requires MongoDB Enterprise Edition and is not available in standard Docker images' + ); + } return { valid: errors.length === 0, errors, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src-tauri/tests/integration/mysql_integration_test.rs(1 hunks)src/features/databases/providers/mongodb.provider.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/features/databases/providers/mongodb.provider.tsx (4)
src/features/databases/registry/database-provider.interface.ts (2)
DatabaseProvider(14-73)FieldsOptions(6-8)src/features/databases/types/form.types.ts (1)
FieldGroup(37-41)src/features/databases/types/docker.types.ts (2)
DockerRunArgs(16-22)ValidationResult(43-46)src/shared/types/container.ts (1)
Container(14-29)
src-tauri/tests/integration/mysql_integration_test.rs (1)
src-tauri/tests/integration/utils.rs (9)
docker_available(9-15)clean_container(18-28)run_docker_command(177-188)wait_for_container_ready(105-138)container_exists(31-47)get_container_status(50-74)clean_volume(156-165)create_volume(141-153)volume_exists(168-174)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend (Tests + Build)
🔇 Additional comments (5)
src-tauri/tests/integration/mysql_integration_test.rs (2)
15-113: LGTM! Test implementation follows best practices.The basic MySQL container test correctly validates command generation, performs robust readiness checks using the retry-based helper, and ensures proper cleanup. The previous hard-coded wait issue has been resolved with
wait_for_container_ready.
115-209: LGTM! Volume test properly handles errors and cleanup.The volume test correctly addresses both previous review concerns:
- Uses
wait_for_container_readyinstead of fixed delays- Fails fast with
panic!if volume creation fails, including cleanup of both container and volumesrc/features/databases/providers/mongodb.provider.tsx (3)
238-241: LGTM! Correct MongoDB flag used.The oplog size flag has been corrected to
--oplogSizeMB(previously was--oplogSize), which is the proper mongod argument.
124-199: Well-structured advanced configuration fields.The field groups are logically organized (Authentication, Replication & Sharding, Storage, Security) with helpful descriptions and tooltips. The helpText on line 172 appropriately warns about inMemory requiring Enterprise Edition.
255-261: LGTM! Connection string format is correct.The connection string correctly includes username, password, host, port, database, and authSource query parameter, which is the standard MongoDB connection URI format.
1 participant
Summary by CodeRabbit
New Features
Improvements
Removed
Tests