feat: Implement dashboard and settings templates with blocklist manag… #20
Conversation
…ement - Updated dashboard.html to include a blocklist table with example data and actions. - Created settings.html for domain management, including blocklists, whitelists, and regex patterns. - Added functionality to manage DNS and HTTP port configurations, upstream servers, and cache size. - Introduced database support for configuration management, including initialization and CRUD operations for domain lists and regex patterns. - Enhanced configuration loading to prioritize database values over defaults.
There was a problem hiding this comment.
Pull Request Overview
This PR implements dashboard and settings templates with comprehensive domain blocklist management functionality. It introduces database-backed configuration storage that allows users to manage DNS settings, domain lists, and regex patterns through a web interface with two theme options (TVA and Cockpit).
Key changes include:
- Added SQLite database support for persistent configuration and domain management
- Created comprehensive settings UI with domain blocklist/whitelist management and regex pattern support
- Enhanced configuration system to prioritize database values over static defaults
- Implemented API endpoints for configuration management with hot-swapping capability
Reviewed Changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/dbconfig/dbconfig.go | New database configuration module with CRUD operations for settings, domain lists, whitelist, and regex patterns |
| internal/config/config.go | Enhanced configuration loading to integrate database values with existing viper/flag system |
| internal/api/templates/settings.html | Comprehensive settings UI template with dual theme support for domain and configuration management |
| internal/api/templates/dashboard.html | Updated dashboard with blocklist table and settings page integration |
| internal/api/static/js/dashboard.js | Added settings page functionality and configuration management features |
| internal/api/server.go | New API endpoints for configuration management and updated page routing |
| internal/api/blocklist_handlers.go | Enhanced handlers to persist domain operations to database |
| go.mod | Added SQLite driver dependency |
| cmd/server/main.go | Updated server initialization to use database-backed configuration |
| func InitDB() (*sql.DB, error) { | ||
| db, err := sql.Open("sqlite3", "goadblock.db") |
There was a problem hiding this comment.
The database file path is hardcoded and will be created in the current working directory. This could lead to permission issues or data loss. Consider making the database path configurable and using a more appropriate location like a data directory.
| func InitDB() (*sql.DB, error) { | |
| db, err := sql.Open("sqlite3", "goadblock.db") | |
| func InitDB(dbPath string) (*sql.DB, error) { | |
| db, err := sql.Open("sqlite3", dbPath) |
| func InitConfig() error { | ||
| //VIPER Priority : flags -> env -> config -> default | ||
| //VIPER Priority : flags -> env -> config -> database -> default | ||
|
|
||
| // Initialize database first | ||
| db, err := dbconfig.InitDB() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to initialize database: %w", err) | ||
| } | ||
| defer db.Close() // Temporary connection for setup |
There was a problem hiding this comment.
Opening and closing database connections in every function call (InitConfig, GetDnsPort, GetHttpPort, etc.) is inefficient. Consider using a connection pool or maintaining a persistent connection that's properly managed.
| viper.SetDefault("config", "") | ||
|
|
||
| // Load values from database and override defaults | ||
| for key, _ := range defaultConfigs { |
There was a problem hiding this comment.
Using blank identifier for unused variable. Replace with 'for key := range defaultConfigs' to be more idiomatic.
| for key, _ := range defaultConfigs { | |
| for key := range defaultConfigs { |
| dns_port: parseInt(this.dnsPort) || 53, | ||
| http_port: parseInt(this.httpPort) || 8080, | ||
| cache_size: parseInt(this.cacheSize) || 10000, | ||
| upstream_servers: this.upstreamServers || '8.8.8.8:53,1.1.1.1:53' |
There was a problem hiding this comment.
The upstreamServers field contains newline-separated values from a textarea, but the fallback and API expect comma-separated values. This mismatch could cause configuration errors.
| upstream_servers: this.upstreamServers || '8.8.8.8:53,1.1.1.1:53' | |
| upstream_servers: (this.upstreamServers | |
| ? this.upstreamServers.replace(/[\r\n]+/g, ',').replace(/\s*,\s*/g, ',').replace(/^,|,$/g, '') | |
| : '8.8.8.8:53,1.1.1.1:53') |
|
|
||
| // Hot-swap ports | ||
| if confUpdate.DNSPort != 0 { | ||
| s.dnsServer.Shutdown(context.Background()) |
There was a problem hiding this comment.
DNS server shutdown errors are not handled. If shutdown fails, the new server may conflict with the old one or resources may not be properly cleaned up.
| go func() { | ||
| if err := s.dnsServer.Start(fmt.Sprintf(":%d", confUpdate.DNSPort)); err != nil { | ||
| log.Fatalf("Failed to start DNS server: %v", err) | ||
| } | ||
| log.Println("DNS server restarted on new port") | ||
| }() |
There was a problem hiding this comment.
Starting the DNS server in a goroutine without proper error handling to the caller could leave the system in an inconsistent state. The API response indicates success even if the DNS server fails to start.
| go func() { | |
| if err := s.dnsServer.Start(fmt.Sprintf(":%d", confUpdate.DNSPort)); err != nil { | |
| log.Fatalf("Failed to start DNS server: %v", err) | |
| } | |
| log.Println("DNS server restarted on new port") | |
| }() | |
| if err := s.dnsServer.Start(fmt.Sprintf(":%d", confUpdate.DNSPort)); err != nil { | |
| log.Printf("Failed to start DNS server: %v", err) | |
| http.Error(w, "Failed to start DNS server", http.StatusInternalServerError) | |
| return | |
| } | |
| log.Println("DNS server restarted on new port") |
…ement