Refactor/architecture optimization #8
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR contains comprehensive architecture optimizations for the FileCodeBox system, upgrading it to version 1.8.2. The main purpose is to modernize the admin interface with a new theme system, improve configuration management with source-based loading, and enhance the static asset serving architecture.
Key changes include:
- Implementation of a modern admin UI theme with dark/light mode support
- Refactoring of configuration management to use a source-based pattern with proper fallback mechanisms
- Enhanced static asset serving with theme fallback capabilities
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| themes/2025/admin/js/main.js | Enhanced admin interface with theme toggling, improved navigation, and modernized mobile menu handling |
| themes/2025/admin/js/config-simple.js | Added configuration normalization and improved UI config handling with better type safety |
| themes/2025/admin/index.html | Complete modernization of admin interface with sidebar navigation, header metrics, and responsive design |
| themes/2025/admin/css/admin-modern.css | New comprehensive CSS framework with theme variables, responsive design, and modern component styling |
| main.go | Version bump to 1.8.2 |
| internal/static/assets.go | Major refactoring of static asset serving with theme fallback system and improved path resolution |
| internal/services/admin/config.go | Enhanced config service with better UI config handling and YAML persistence |
| internal/routes/admin.go | Simplified admin route handling using new theme resolution system |
| internal/models/web/admin.go | Added support for notify fields in admin config requests |
| internal/models/service/system.go | Version update to 1.8.2 |
| internal/handlers/api.go | Version update in health response |
| internal/handlers/admin.go | Added notify field support in config responses |
| internal/config/source.go | New configuration source system with YAML and environment variable support |
| internal/config/manager_test.go | Added tests for new source-based configuration system |
| internal/config/manager.go | Refactored configuration loading to use source-based pattern |
| docs/swagger.yaml | Version update in API documentation |
| docs/swagger.json | Version update in API documentation |
| docs/swagger-enhanced.yaml | Version update in enhanced API documentation |
| config.yaml | Updated default configuration with proper structure |
| VERSION | Version bump to 1.8.2 |
| const clone = typeof structuredClone === 'function' | ||
| ? structuredClone(config) | ||
| : JSON.parse(JSON.stringify(config)); |
There was a problem hiding this comment.
Using JSON.parse(JSON.stringify()) as a fallback for deep cloning is inefficient and can lose function properties, dates, and other non-JSON serializable values. Consider implementing a proper deep clone function or using a library like Lodash's cloneDeep for better performance and correctness.
| func resolveThemeFilePath(cfg *config.ConfigManager, parts ...string) (string, error) { | ||
| var firstErr error | ||
| for _, candidate := range themeCandidates(cfg) { | ||
| pathParts := append([]string{candidate}, parts...) | ||
| path := filepath.Join(pathParts...) | ||
| info, err := os.Stat(path) | ||
| if err == nil { | ||
| if !info.IsDir() { | ||
| return path, nil | ||
| } | ||
| continue | ||
| } | ||
| if firstErr == nil { | ||
| firstErr = err | ||
| } | ||
| } | ||
| if firstErr == nil { | ||
| firstErr = os.ErrNotExist | ||
| } | ||
| return "", firstErr |
There was a problem hiding this comment.
The function skips directories with continue but doesn't handle the case where a directory is expected. If the requested path should be a directory, this logic will incorrectly treat it as not found. Consider adding a parameter to specify whether the target should be a file or directory.
| func parseBool(val string) (bool, error) { | ||
| switch strings.ToLower(strings.TrimSpace(val)) { | ||
| case "1", "true", "t", "yes", "y": | ||
| return true, nil | ||
| case "0", "false", "f", "no", "n": | ||
| return false, nil | ||
| default: | ||
| return false, fmt.Errorf("invalid boolean value %q", val) | ||
| } | ||
| } |
There was a problem hiding this comment.
This custom boolean parsing duplicates functionality available in Go's standard library. Consider using strconv.ParseBool() which is more standard and well-tested, or clearly document why this custom implementation with additional accepted values ('y', 'n', 't', 'f') is necessary.
No description provided.