fix(review): address CodeRabbit findings - PrevSection logic, Branch() empty guard, isEncrypted dedup, DISK_PLACEHOLDER constant, regex pacman uncomment, UEFI-safe limine hook
- commit
- fad48414639fc1d25a94a0e3f899b22571631ff6
- author
- brice · · 2026-02-27 05:47 UTC
- parent
- 83effce
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
7 files · +35 -17
M install/configs/limine-update.hook +1 -1
| @@ -11,5 +11,5 @@ Target = linux-cachyos | |||
| 11 | 11 | [Action] | |
| 12 | 12 | Description = Updating Limine bootloader after kernel upgrade... | |
| 13 | 13 | When = PostTransaction | |
| 14 | - | Exec = /usr/bin/limine bios-install DISK_PLACEHOLDER | |
| 14 | + | Exec = /bin/sh -c '[ -d /sys/firmware/efi ] || /usr/bin/limine bios-install DISK_PLACEHOLDER' | |
| 15 | 15 | Depends = limine | |
| @@ -11,5 +11,5 @@ Target = linux-cachyos | |||||
| 11 | [Action] | 11 | [Action] | ||
| 12 | Description = Updating Limine bootloader after kernel upgrade... | 12 | Description = Updating Limine bootloader after kernel upgrade... | ||
| 13 | When = PostTransaction | 13 | When = PostTransaction | ||
| 14 | - | Exec = /usr/bin/limine bios-install DISK_PLACEHOLDER | 14 | + | Exec = /bin/sh -c '[ -d /sys/firmware/efi ] || /usr/bin/limine bios-install DISK_PLACEHOLDER' |
| 15 | Depends = limine | 15 | Depends = limine | ||
M internal/application/handlers/bootstrap_handler.go +7 -2
| @@ -86,9 +86,14 @@ func (h *BootstrapHandler) cloneRepo(ctx context.Context) error { | |||
| 86 | 86 | return nil | |
| 87 | 87 | } | |
| 88 | 88 | ||
| 89 | - | h.logger.Info("Cloning repository", "url", h.repoURL) | |
| 89 | + | branch := h.branch | |
| 90 | + | if branch == "" { | |
| 91 | + | branch = "main" | |
| 92 | + | } | |
| 93 | + | ||
| 94 | + | h.logger.Info("Cloning repository", "url", h.repoURL, "branch", branch) | |
| 90 | 95 | ||
| 91 | - | cmd := exec.CommandContext(ctx, "git", "clone", "--depth", "1", "--branch", h.branch, h.repoURL, repoDir) | |
| 96 | + | cmd := exec.CommandContext(ctx, "git", "clone", "--depth", "1", "--branch", branch, h.repoURL, repoDir) | |
| 92 | 97 | if output, err := cmd.CombinedOutput(); err != nil { | |
| 93 | 98 | return fmt.Errorf("git clone failed: %w (output: %s)", err, string(output)) | |
| 94 | 99 | } | |
| @@ -86,9 +86,14 @@ func (h *BootstrapHandler) cloneRepo(ctx context.Context) error { | |||||
| 86 | return nil | 86 | return nil | ||
| 87 | } | 87 | } | ||
| 88 | 88 | ||||
| 89 | - | h.logger.Info("Cloning repository", "url", h.repoURL) | 89 | + | branch := h.branch |
| 90 | + | if branch == "" { | |||
| 91 | + | branch = "main" | |||
| 92 | + | } | |||
| 93 | + | ||||
| 94 | + | h.logger.Info("Cloning repository", "url", h.repoURL, "branch", branch) | |||
| 90 | 95 | ||||
| 91 | - | cmd := exec.CommandContext(ctx, "git", "clone", "--depth", "1", "--branch", h.branch, h.repoURL, repoDir) | 96 | + | cmd := exec.CommandContext(ctx, "git", "clone", "--depth", "1", "--branch", branch, h.repoURL, repoDir) |
| 92 | if output, err := cmd.CombinedOutput(); err != nil { | 97 | if output, err := cmd.CombinedOutput(); err != nil { | ||
| 93 | return fmt.Errorf("git clone failed: %w (output: %s)", err, string(output)) | 98 | return fmt.Errorf("git clone failed: %w (output: %s)", err, string(output)) | ||
| 94 | } | 99 | } | ||
M internal/application/handlers/post_install_handler.go +11 -6
| @@ -257,6 +257,9 @@ func (h *PostInstallHandler) setupSnapperSync(ctx context.Context, mountPoint st | |||
| 257 | 257 | return nil | |
| 258 | 258 | } | |
| 259 | 259 | ||
| 260 | + | // limineDiskPlaceholder is the placeholder in limine-update.hook replaced at install time. | |
| 261 | + | const limineDiskPlaceholder = "DISK_PLACEHOLDER" | |
| 262 | + | ||
| 260 | 263 | func (h *PostInstallHandler) installLimineHook(mountPoint, targetDisk string) error { | |
| 261 | 264 | hooksDir := filepath.Join(mountPoint, "etc", "pacman.d", "hooks") | |
| 262 | 265 | if err := h.fs.MkdirAll(hooksDir, 0755); err != nil { | |
| @@ -269,7 +272,7 @@ func (h *PostInstallHandler) installLimineHook(mountPoint, targetDisk string) er | |||
| 269 | 272 | return fmt.Errorf("failed to get limine hook template: %w", err) | |
| 270 | 273 | } | |
| 271 | 274 | } | |
| 272 | - | hookContent := strings.ReplaceAll(string(content), "DISK_PLACEHOLDER", targetDisk) | |
| 275 | + | hookContent := strings.ReplaceAll(string(content), limineDiskPlaceholder, targetDisk) | |
| 273 | 276 | return h.fs.WriteFile(filepath.Join(hooksDir, "limine-update.hook"), []byte(hookContent), 0644) | |
| 274 | 277 | } | |
| 275 | 278 | ||
| @@ -280,14 +283,16 @@ func (h *PostInstallHandler) tunePacmanConfig(mountPoint string) error { | |||
| 280 | 283 | return fmt.Errorf("failed to read pacman.conf: %w", err) | |
| 281 | 284 | } | |
| 282 | 285 | conf := string(content) | |
| 283 | - | conf = enablePacmanOption(conf, "#Color", "Color") | |
| 284 | - | conf = enablePacmanOption(conf, "#ParallelDownloads = 5", "ParallelDownloads = 5") | |
| 285 | - | conf = enablePacmanOption(conf, "#ILoveCandy", "ILoveCandy") | |
| 286 | + | conf = uncommentPacmanOption(conf, "Color") | |
| 287 | + | conf = uncommentPacmanOption(conf, "ParallelDownloads") | |
| 288 | + | conf = uncommentPacmanOption(conf, "ILoveCandy") | |
| 286 | 289 | return h.fs.WriteFile(confPath, []byte(conf), 0644) | |
| 287 | 290 | } | |
| 288 | 291 | ||
| 289 | - | func enablePacmanOption(conf, commented, replacement string) string { | |
| 290 | - | return strings.ReplaceAll(conf, commented, replacement) | |
| 292 | + | // uncommentPacmanOption uncomments a pacman.conf option, handling optional spaces after #. | |
| 293 | + | func uncommentPacmanOption(conf, option string) string { | |
| 294 | + | re := regexp.MustCompile(`(?m)^#\s*(` + regexp.QuoteMeta(option) + `.*)$`) | |
| 295 | + | return re.ReplaceAllString(conf, "$1") | |
| 291 | 296 | } | |
| 292 | 297 | ||
| 293 | 298 | func (h *PostInstallHandler) verifyInstallation(mountPoint string, encrypted bool) []string { | |
| @@ -257,6 +257,9 @@ func (h *PostInstallHandler) setupSnapperSync(ctx context.Context, mountPoint st | |||||
| 257 | return nil | 257 | return nil | ||
| 258 | } | 258 | } | ||
| 259 | 259 | ||||
| 260 | + | // limineDiskPlaceholder is the placeholder in limine-update.hook replaced at install time. | |||
| 261 | + | const limineDiskPlaceholder = "DISK_PLACEHOLDER" | |||
| 262 | + | ||||
| 260 | func (h *PostInstallHandler) installLimineHook(mountPoint, targetDisk string) error { | 263 | func (h *PostInstallHandler) installLimineHook(mountPoint, targetDisk string) error { | ||
| 261 | hooksDir := filepath.Join(mountPoint, "etc", "pacman.d", "hooks") | 264 | hooksDir := filepath.Join(mountPoint, "etc", "pacman.d", "hooks") | ||
| 262 | if err := h.fs.MkdirAll(hooksDir, 0755); err != nil { | 265 | if err := h.fs.MkdirAll(hooksDir, 0755); err != nil { | ||
| @@ -269,7 +272,7 @@ func (h *PostInstallHandler) installLimineHook(mountPoint, targetDisk string) er | |||||
| 269 | return fmt.Errorf("failed to get limine hook template: %w", err) | 272 | return fmt.Errorf("failed to get limine hook template: %w", err) | ||
| 270 | } | 273 | } | ||
| 271 | } | 274 | } | ||
| 272 | - | hookContent := strings.ReplaceAll(string(content), "DISK_PLACEHOLDER", targetDisk) | 275 | + | hookContent := strings.ReplaceAll(string(content), limineDiskPlaceholder, targetDisk) |
| 273 | return h.fs.WriteFile(filepath.Join(hooksDir, "limine-update.hook"), []byte(hookContent), 0644) | 276 | return h.fs.WriteFile(filepath.Join(hooksDir, "limine-update.hook"), []byte(hookContent), 0644) | ||
| 274 | } | 277 | } | ||
| 275 | 278 | ||||
| @@ -280,14 +283,16 @@ func (h *PostInstallHandler) tunePacmanConfig(mountPoint string) error { | |||||
| 280 | return fmt.Errorf("failed to read pacman.conf: %w", err) | 283 | return fmt.Errorf("failed to read pacman.conf: %w", err) | ||
| 281 | } | 284 | } | ||
| 282 | conf := string(content) | 285 | conf := string(content) | ||
| 283 | - | conf = enablePacmanOption(conf, "#Color", "Color") | 286 | + | conf = uncommentPacmanOption(conf, "Color") |
| 284 | - | conf = enablePacmanOption(conf, "#ParallelDownloads = 5", "ParallelDownloads = 5") | 287 | + | conf = uncommentPacmanOption(conf, "ParallelDownloads") |
| 285 | - | conf = enablePacmanOption(conf, "#ILoveCandy", "ILoveCandy") | 288 | + | conf = uncommentPacmanOption(conf, "ILoveCandy") |
| 286 | return h.fs.WriteFile(confPath, []byte(conf), 0644) | 289 | return h.fs.WriteFile(confPath, []byte(conf), 0644) | ||
| 287 | } | 290 | } | ||
| 288 | 291 | ||||
| 289 | - | func enablePacmanOption(conf, commented, replacement string) string { | 292 | + | // uncommentPacmanOption uncomments a pacman.conf option, handling optional spaces after #. |
| 290 | - | return strings.ReplaceAll(conf, commented, replacement) | 293 | + | func uncommentPacmanOption(conf, option string) string { |
| 294 | + | re := regexp.MustCompile(`(?m)^#\s*(` + regexp.QuoteMeta(option) + `.*)$`) | |||
| 295 | + | return re.ReplaceAllString(conf, "$1") | |||
| 291 | } | 296 | } | ||
| 292 | 297 | ||||
| 293 | func (h *PostInstallHandler) verifyInstallation(mountPoint string, encrypted bool) []string { | 298 | func (h *PostInstallHandler) verifyInstallation(mountPoint string, encrypted bool) []string { | ||
M internal/config/config.go +8 -4
| @@ -293,13 +293,17 @@ func NewConfig(version string) *Config { | |||
| 293 | 293 | ||
| 294 | 294 | // Branch returns the git branch to use for asset downloads and repo cloning. | |
| 295 | 295 | func (c *Config) Branch() string { | |
| 296 | - | rawURL := c.RawURL | |
| 296 | + | rawURL := strings.TrimSpace(c.RawURL) | |
| 297 | + | if rawURL == "" { | |
| 298 | + | return "main" | |
| 299 | + | } | |
| 297 | 300 | // Extract branch from RawURL: "https://raw.githubusercontent.com/bnema/archup/<branch>" | |
| 298 | 301 | parts := strings.Split(rawURL, "/") | |
| 299 | - | if len(parts) > 0 { | |
| 300 | - | return parts[len(parts)-1] | |
| 302 | + | last := parts[len(parts)-1] | |
| 303 | + | if last == "" { | |
| 304 | + | return "main" | |
| 301 | 305 | } | |
| 302 | - | return "main" | |
| 306 | + | return last | |
| 303 | 307 | } | |
| 304 | 308 | ||
| 305 | 309 | // Load reads configuration from a shell-compatible config file | |
| @@ -293,13 +293,17 @@ func NewConfig(version string) *Config { | |||||
| 293 | 293 | ||||
| 294 | // Branch returns the git branch to use for asset downloads and repo cloning. | 294 | // Branch returns the git branch to use for asset downloads and repo cloning. | ||
| 295 | func (c *Config) Branch() string { | 295 | func (c *Config) Branch() string { | ||
| 296 | - | rawURL := c.RawURL | 296 | + | rawURL := strings.TrimSpace(c.RawURL) |
| 297 | + | if rawURL == "" { | |||
| 298 | + | return "main" | |||
| 299 | + | } | |||
| 297 | // Extract branch from RawURL: "https://raw.githubusercontent.com/bnema/archup/<branch>" | 300 | // Extract branch from RawURL: "https://raw.githubusercontent.com/bnema/archup/<branch>" | ||
| 298 | parts := strings.Split(rawURL, "/") | 301 | parts := strings.Split(rawURL, "/") | ||
| 299 | - | if len(parts) > 0 { | 302 | + | last := parts[len(parts)-1] |
| 300 | - | return parts[len(parts)-1] | 303 | + | if last == "" { |
| 304 | + | return "main" | |||
| 301 | } | 305 | } | ||
| 302 | - | return "main" | 306 | + | return last |
| 303 | } | 307 | } | ||
| 304 | 308 | ||||
| 305 | // Load reads configuration from a shell-compatible config file | 309 | // Load reads configuration from a shell-compatible config file | ||
M internal/interfaces/tui/app.go +4 -1
| @@ -373,9 +373,12 @@ func (a *App) handleReposInput(msg tea.KeyMsg) (tea.Model, tea.Cmd) { | |||
| 373 | 373 | case "down": | |
| 374 | 374 | a.reposModel.MoveDown() | |
| 375 | 375 | return a, nil | |
| 376 | - | case "tab", "shift+tab": | |
| 376 | + | case "tab": | |
| 377 | 377 | a.reposModel.NextSection() | |
| 378 | 378 | return a, nil | |
| 379 | + | case "shift+tab": | |
| 380 | + | a.reposModel.PrevSection() | |
| 381 | + | return a, nil | |
| 379 | 382 | case "enter": | |
| 380 | 383 | a.formData.AURHelper = a.reposModel.SelectedAURHelper() | |
| 381 | 384 | a.formData.EnableChaotic = a.reposModel.SelectedChaoticEnabled() | |
| @@ -373,9 +373,12 @@ func (a *App) handleReposInput(msg tea.KeyMsg) (tea.Model, tea.Cmd) { | |||||
| 373 | case "down": | 373 | case "down": | ||
| 374 | a.reposModel.MoveDown() | 374 | a.reposModel.MoveDown() | ||
| 375 | return a, nil | 375 | return a, nil | ||
| 376 | - | case "tab", "shift+tab": | 376 | + | case "tab": |
| 377 | a.reposModel.NextSection() | 377 | a.reposModel.NextSection() | ||
| 378 | return a, nil | 378 | return a, nil | ||
| 379 | + | case "shift+tab": | |||
| 380 | + | a.reposModel.PrevSection() | |||
| 381 | + | return a, nil | |||
| 379 | case "enter": | 382 | case "enter": | ||
| 380 | a.formData.AURHelper = a.reposModel.SelectedAURHelper() | 383 | a.formData.AURHelper = a.reposModel.SelectedAURHelper() | ||
| 381 | a.formData.EnableChaotic = a.reposModel.SelectedChaoticEnabled() | 384 | a.formData.EnableChaotic = a.reposModel.SelectedChaoticEnabled() | ||
M internal/interfaces/tui/handlers/install_handler.go +3 -2
| @@ -117,6 +117,7 @@ func CreateInstallationCommand(app AppContext, formData models.FormData) tea.Cmd | |||
| 117 | 117 | } | |
| 118 | 118 | ||
| 119 | 119 | // Phase 2: Partition disk with chosen encryption using user password | |
| 120 | + | isEncrypted := parseEncryptionType(formData.EncryptionType) != disk.EncryptionTypeNone | |
| 120 | 121 | partitionCmd := commands.PartitionDiskCommand{ | |
| 121 | 122 | TargetDisk: formData.TargetDisk, | |
| 122 | 123 | RootSizeGB: 0, // Use all available space | |
| @@ -141,7 +142,7 @@ func CreateInstallationCommand(app AppContext, formData models.FormData) tea.Cmd | |||
| 141 | 142 | MountPoint: "/mnt", | |
| 142 | 143 | KernelVariant: parseKernelVariant(formData.KernelVariant), | |
| 143 | 144 | IncludeMicrocode: formData.Microcode, | |
| 144 | - | Encrypted: parseEncryptionType(formData.EncryptionType) != disk.EncryptionTypeNone, | |
| 145 | + | Encrypted: isEncrypted, | |
| 145 | 146 | } | |
| 146 | 147 | if _, err := svc.RunBaseInstall(ctx, baseCmd); err != nil { | |
| 147 | 148 | logger.Error("Base installation failed", "error", err) | |
| @@ -217,7 +218,7 @@ func CreateInstallationCommand(app AppContext, formData models.FormData) tea.Cmd | |||
| 217 | 218 | RunPostBootScripts: true, | |
| 218 | 219 | InstallDankLinux: formData.InstallDankLinux, | |
| 219 | 220 | TargetDisk: formData.TargetDisk, | |
| 220 | - | Encrypted: parseEncryptionType(formData.EncryptionType) != disk.EncryptionTypeNone, | |
| 221 | + | Encrypted: isEncrypted, | |
| 221 | 222 | } | |
| 222 | 223 | if _, err := svc.RunPostInstall(ctx, postCmd); err != nil { | |
| 223 | 224 | logger.Error("Post-installation failed", "error", err) | |
| @@ -117,6 +117,7 @@ func CreateInstallationCommand(app AppContext, formData models.FormData) tea.Cmd | |||||
| 117 | } | 117 | } | ||
| 118 | 118 | ||||
| 119 | // Phase 2: Partition disk with chosen encryption using user password | 119 | // Phase 2: Partition disk with chosen encryption using user password | ||
| 120 | + | isEncrypted := parseEncryptionType(formData.EncryptionType) != disk.EncryptionTypeNone | |||
| 120 | partitionCmd := commands.PartitionDiskCommand{ | 121 | partitionCmd := commands.PartitionDiskCommand{ | ||
| 121 | TargetDisk: formData.TargetDisk, | 122 | TargetDisk: formData.TargetDisk, | ||
| 122 | RootSizeGB: 0, // Use all available space | 123 | RootSizeGB: 0, // Use all available space | ||
| @@ -141,7 +142,7 @@ func CreateInstallationCommand(app AppContext, formData models.FormData) tea.Cmd | |||||
| 141 | MountPoint: "/mnt", | 142 | MountPoint: "/mnt", | ||
| 142 | KernelVariant: parseKernelVariant(formData.KernelVariant), | 143 | KernelVariant: parseKernelVariant(formData.KernelVariant), | ||
| 143 | IncludeMicrocode: formData.Microcode, | 144 | IncludeMicrocode: formData.Microcode, | ||
| 144 | - | Encrypted: parseEncryptionType(formData.EncryptionType) != disk.EncryptionTypeNone, | 145 | + | Encrypted: isEncrypted, |
| 145 | } | 146 | } | ||
| 146 | if _, err := svc.RunBaseInstall(ctx, baseCmd); err != nil { | 147 | if _, err := svc.RunBaseInstall(ctx, baseCmd); err != nil { | ||
| 147 | logger.Error("Base installation failed", "error", err) | 148 | logger.Error("Base installation failed", "error", err) | ||
| @@ -217,7 +218,7 @@ func CreateInstallationCommand(app AppContext, formData models.FormData) tea.Cmd | |||||
| 217 | RunPostBootScripts: true, | 218 | RunPostBootScripts: true, | ||
| 218 | InstallDankLinux: formData.InstallDankLinux, | 219 | InstallDankLinux: formData.InstallDankLinux, | ||
| 219 | TargetDisk: formData.TargetDisk, | 220 | TargetDisk: formData.TargetDisk, | ||
| 220 | - | Encrypted: parseEncryptionType(formData.EncryptionType) != disk.EncryptionTypeNone, | 221 | + | Encrypted: isEncrypted, |
| 221 | } | 222 | } | ||
| 222 | if _, err := svc.RunPostInstall(ctx, postCmd); err != nil { | 223 | if _, err := svc.RunPostInstall(ctx, postCmd); err != nil { | ||
| 223 | logger.Error("Post-installation failed", "error", err) | 224 | logger.Error("Post-installation failed", "error", err) | ||
M internal/interfaces/tui/models/repos.go +1 -1
| @@ -143,5 +143,5 @@ func (rm *ReposModelImpl) NextSection() { | |||
| 143 | 143 | ||
| 144 | 144 | // PrevSection moves focus to the previous section. | |
| 145 | 145 | func (rm *ReposModelImpl) PrevSection() { | |
| 146 | - | rm.focusSection = (rm.focusSection + 1) % 2 | |
| 146 | + | rm.focusSection = (rm.focusSection - 1 + 2) % 2 | |
| 147 | 147 | } | |
| @@ -143,5 +143,5 @@ func (rm *ReposModelImpl) NextSection() { | |||||
| 143 | 143 | ||||
| 144 | // PrevSection moves focus to the previous section. | 144 | // PrevSection moves focus to the previous section. | ||
| 145 | func (rm *ReposModelImpl) PrevSection() { | 145 | func (rm *ReposModelImpl) PrevSection() { | ||
| 146 | - | rm.focusSection = (rm.focusSection + 1) % 2 | 146 | + | rm.focusSection = (rm.focusSection - 1 + 2) % 2 |
| 147 | } | 147 | } | ||