Skip to content

feat: add interactive ui advanced#6

Merged
Evan-Paillard merged 12 commits intomainfrom
feat/addInteractiveUiadvanced
Mar 16, 2026
Merged

feat: add interactive ui advanced#6
Evan-Paillard merged 12 commits intomainfrom
feat/addInteractiveUiadvanced

Conversation

@Evan-Paillard
Copy link
Copy Markdown
Collaborator

@Evan-Paillard Evan-Paillard commented Mar 12, 2026

Add advanced TUI mode

Description

Add advanced mode flow and persist TUI state across screen transitions.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Summary by CodeRabbit

  • New Features
    • New wizard screens: target architecture, file injection manager (add/edit/remove), init mode (default or custom file), and compression options.
    • Quick and Advanced modes with expanded navigation in Advanced to step through architecture → inject → init → compression → summary.
    • Summary now shows chosen mode, init selection, and injections; CLI/build incorporate these selections.
  • Usability
    • Improved help prompts and per-screen key handling; default host architecture detected.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 12, 2026

📝 Walkthrough

Walkthrough

Adds an advanced multi-step wizard to the TUI: Architecture, Inject, Init, and Compression screens, new WizardMode (Quick/Advanced), BuildConfig extensions (injections, init_mode), screen sync/validation, updated build/CLI generation to include injections and custom init, and per-screen event handlers.

Changes

Cohort / File(s) Summary
Core App & Config
src/tui/app.rs
Added WizardMode, new Screen variants, BuildConfig (public fields: injections, init_mode, arch, output), host arch detection, output extension helpers, shell_quote, enter_advanced_mode(), screen sync on enter/exit, validation helpers, and CLI/build logic updated to include injections and init.
Event Routing & Key Handling
src/tui/events.rs
Routed new screens to per-screen handlers (handle_arch_keys, handle_inject_keys, handle_init_keys, handle_compress_keys), adjusted quit semantics for text-input screens, and added key to enter Advanced mode from Summary.
Screens — Architecture
src/tui/screens/arch.rs
New ArchScreen with selection/toggle, new_with_default/sync_from_config/get_selected, and draw routine for amd64/arm64.
Screens — Inject
src/tui/screens/inject.rs
New InjectScreen and Injection type: list management, in-place editing (start/cancel/confirm), input handling, delete/toggle, get_injections(), and draw UI.
Screens — Init
src/tui/screens/init.rs
New InitScreen and InitMode (Default, CustomFile(PathBuf)): selection, path_input, toggle, get_init_mode(), validation, and conditional draw for file path input.
Screens — Compression
src/tui/screens/compress.rs
New CompressScreen: options (gzip/zstd/none), navigation (next/prev), get_selected(), and draw routine.
Screens Module & Exports
src/tui/screens/mod.rs
Added modules arch, inject, init, compress and re-exported corresponding screen types.
Summary & UI Rendering
src/tui/screens/summary.rs, src/tui/ui.rs
Summary now includes Init mode, injections, and mode label; UI dispatch and help text updated for new screens and WizardMode.
Misc / Manifest
Cargo.toml, manifest_file
Small manifest/metadata edits reported by analyzer.
sequenceDiagram
    participant User
    participant App
    participant Screens as Arch/Inject/Init/Compress
    participant Config as BuildConfig
    participant Builder as InitramfsBuilder

    User->>App: Start TUI
    App->>App: Default Quick flow (Language → Image → Summary)
    User->>App: Press 'a' (enter advanced)
    App->>App: enter_advanced_mode()
    App->>Screens: Show Architecture
    User->>Screens: Configure Arch, Inject, Init, Compression
    Screens->>Config: sync fields (arch, injections, init_mode, compression)
    User->>App: Proceed to Summary / Build
    App->>Config: validate config
    App->>Builder: start_build(image, arch, injections, init_mode, compression)
    Builder-->>App: build complete / result
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hopped through screens both new and bright,
Arch, inject, init—each choice took light,
Advanced mode opened a winding trail,
Compression set, the build set sail! 🚀

🚥 Pre-merge checks | ❌ 3

❌ Failed checks (1 warning, 2 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'feat: add interactive ui advanced' is vague and does not clearly describe the main changes; it lacks specificity about what advanced UI features were added. Replace with a more descriptive title that specifies the UI feature, e.g., 'feat: add advanced wizard mode with architecture, injection, and init screens'.
Description check ❓ Inconclusive The PR description is minimal and lacks critical details about the implementation, testing approach, and context for the changes. Expand the description to include: what problem the advanced mode solves, how TUI state is persisted, testing methodology, and how to verify the feature works end-to-end.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/addInteractiveUiadvanced
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/tui/app.rs (2)

127-149: ⚠️ Potential issue | 🟠 Major

Persist screen state on every non-forward transition.

Line 127 and Line 145 switch screens without the sync_screen_on_exit() step that next_screen() already uses. In the advanced flow that can silently drop edits: a custom image entered before enter_advanced_mode() stays stale in config, and an architecture change can be reset after Back → forward because sync_screen_on_enter() reloads it from the old config value.

Suggested fix
 pub fn prev_screen(&mut self) {
     self.validation_error = None;
+    self.sync_screen_on_exit();
     self.screen = match self.screen {
         Screen::Language => Screen::Language,
         Screen::Image => Screen::Language,
         Screen::Architecture => Screen::Image,
         Screen::Inject => Screen::Architecture,
@@
     }
     self.sync_screen_on_enter();
 }

 pub fn enter_advanced_mode(&mut self) {
+    self.sync_screen_on_exit();
     self.mode = WizardMode::Advanced;
     self.screen = Screen::Architecture;
     self.sync_screen_on_enter();
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tui/app.rs` around lines 127 - 149, The prev_screen and
enter_advanced_mode flows are switching screens without persisting current
edits; call self.sync_screen_on_exit() before modifying self.screen (and before
changing self.mode in enter_advanced_mode) so the current UI state is saved to
config like next_screen() does; update prev_screen to invoke
self.sync_screen_on_exit() immediately before the match that assigns
self.screen, and update enter_advanced_mode to call self.sync_screen_on_exit()
before setting self.mode = WizardMode::Advanced and self.screen =
Screen::Architecture.

1-297: ⚠️ Potential issue | 🟡 Minor

Run cargo fmt on this file before merging.

CI is already failing on src/tui/app.rs for rustfmt, so this needs formatting cleanup before the PR can land.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tui/app.rs` around lines 1 - 297, The file fails rustfmt check; run
"cargo fmt" (or rustfmt) on the repository to reformat src/tui/app.rs (which
contains App, BuildConfig, detect_host_arch, start_build, generate_cli_command,
etc.) so imports, spacing, and line-wrapping match rustfmt rules, then commit
the formatted changes; no logic changes are needed—just apply rustfmt to this
file (or the whole workspace) before merging.
🧹 Nitpick comments (1)
src/tui/screens/arch.rs (1)

12-28: Consider extracting the arch-to-index mapping to reduce duplication.

new_with_default and sync_from_config contain identical logic for mapping architecture strings to indices.

Proposed refactor
 impl ArchScreen {
+    fn arch_to_index(arch: &str) -> usize {
+        match arch {
+            "arm64" => 1,
+            _ => 0,
+        }
+    }
+
     pub fn new_with_default(arch: &str) -> Self {
-        let selected = match arch {
-            "arm64" => 1,
-            _ => 0,
-        };
         Self {
-            selected,
+            selected: Self::arch_to_index(arch),
             options: ["amd64", "arm64"],
         }
     }

     pub fn sync_from_config(&mut self, arch: &str) {
-        self.selected = match arch {
-            "arm64" => 1,
-            _ => 0,
-        };
+        self.selected = Self::arch_to_index(arch);
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tui/screens/arch.rs` around lines 12 - 28, Both new_with_default and
sync_from_config duplicate the same arch-to-index matching logic; extract that
into a single private helper (e.g., fn arch_to_index(arch: &str) -> usize) and
call it from new_with_default and sync_from_config to compute selected. Update
new_with_default to set selected = Self::arch_to_index(arch) and
sync_from_config to set self.selected = Self::arch_to_index(arch), keeping the
options = ["amd64", "arm64"] initialization unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/tui/app.rs`:
- Around line 280-295: The generated CLI string in generate_cli_command
currently concatenates raw path/argument values (from self.config.injections ->
inj.src/inj.dest, self.config.init_mode when InitMode::CustomFile(path), and
self.config.output) which will break if they contain spaces or shell-sensitive
characters; update generate_cli_command to properly shell-quote or escape those
values before appending (e.g., wrap with a safe quoting function or escape
spaces/quotes/backslashes), ensuring --inject arguments, the --init path and the
-o output are quoted/escaped consistently while preserving the existing
formatting and line continuations.
- Around line 43-47: The default output filename is hardcoded to
"initramfs.cpio.gz" which can mismatch config.compression; update the
initializer that sets output (where the struct with arch, injections, init_mode,
compression, output is created) to derive the extension from Compression (e.g.,
Compression::Gzip -> ".cpio.gz", None -> ".cpio"), and also update the code path
that changes config.compression (the place noted as "line 167" where
config.compression is set) to synchronize config.output accordingly so the
filename extension always reflects the current config.compression value.
- Around line 234-236: The InitMode::CustomFile branch currently passes any
PathBuf into builder.init_script(path) and lets the builder later fail during
the async task; instead validate the path synchronously before spawning: check
that when InitMode::CustomFile(path) is matched the path.exists() &&
path.is_file() (or use equivalent std::fs metadata checks) and return or surface
an immediate error to the UI if the check fails, otherwise call
builder.init_script(path) and then proceed to tokio::spawn; update the code
around the InitMode::CustomFile match and the call sites that handle
builder.init_script to propagate a clear error to the init screen rather than
deferring to the builder.

In `@src/tui/screens/init.rs`:
- Around line 1-4: This file has formatting inconsistencies in the imports
(e.g., the use of ratatui::{ prelude::*, widgets::{Block, Borders, List,
ListItem, ListState, Paragraph}, };) — run `cargo fmt` to reformat the file so
the import blocks and spacing conform to rustfmt rules and CI will pass; commit
the resulting changes.
- Around line 30-36: get_init_mode currently constructs
InitMode::CustomFile(PathBuf::from(&self.path_input)) even when path_input is
empty; change get_init_mode to return Option<InitMode> (or Result if you prefer
validation errors) and return None when selected == 1 and
self.path_input.trim().is_empty(), otherwise return
Some(InitMode::CustomFile(PathBuf::from(&self.path_input))) for the custom case
and Some(InitMode::Default) when selected == 0; update callers of get_init_mode
to handle the Option/Result accordingly.

In `@src/tui/ui.rs`:
- Line 1: This file fails rustfmt formatting; run rustfmt (e.g., cargo fmt) to
reformat the file so imports and code conform to project style — specifically
reformat the top-level use line that references App, Screen, and WizardMode in
src/tui/ui.rs (symbols: App, Screen, WizardMode) so the file passes CI
formatting checks.

---

Outside diff comments:
In `@src/tui/app.rs`:
- Around line 127-149: The prev_screen and enter_advanced_mode flows are
switching screens without persisting current edits; call
self.sync_screen_on_exit() before modifying self.screen (and before changing
self.mode in enter_advanced_mode) so the current UI state is saved to config
like next_screen() does; update prev_screen to invoke self.sync_screen_on_exit()
immediately before the match that assigns self.screen, and update
enter_advanced_mode to call self.sync_screen_on_exit() before setting self.mode
= WizardMode::Advanced and self.screen = Screen::Architecture.
- Around line 1-297: The file fails rustfmt check; run "cargo fmt" (or rustfmt)
on the repository to reformat src/tui/app.rs (which contains App, BuildConfig,
detect_host_arch, start_build, generate_cli_command, etc.) so imports, spacing,
and line-wrapping match rustfmt rules, then commit the formatted changes; no
logic changes are needed—just apply rustfmt to this file (or the whole
workspace) before merging.

---

Nitpick comments:
In `@src/tui/screens/arch.rs`:
- Around line 12-28: Both new_with_default and sync_from_config duplicate the
same arch-to-index matching logic; extract that into a single private helper
(e.g., fn arch_to_index(arch: &str) -> usize) and call it from new_with_default
and sync_from_config to compute selected. Update new_with_default to set
selected = Self::arch_to_index(arch) and sync_from_config to set self.selected =
Self::arch_to_index(arch), keeping the options = ["amd64", "arm64"]
initialization unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fda7699c-f6d7-4021-a1df-54022a5e16a9

📥 Commits

Reviewing files that changed from the base of the PR and between 02cf29d and 508b146.

📒 Files selected for processing (9)
  • src/tui/app.rs
  • src/tui/events.rs
  • src/tui/screens/arch.rs
  • src/tui/screens/compress.rs
  • src/tui/screens/init.rs
  • src/tui/screens/inject.rs
  • src/tui/screens/mod.rs
  • src/tui/screens/summary.rs
  • src/tui/ui.rs

Comment on lines +1 to +4
use ratatui::{
prelude::*,
widgets::{Block, Borders, List, ListItem, ListState, Paragraph},
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Run cargo fmt to fix formatting.

The CI pipeline reports formatting issues in this file. Please run cargo fmt to ensure consistent formatting.

🧰 Tools
🪛 GitHub Actions: CI

[error] 1-1: Rust fmt check failed. Detected formatting changes in src/tui/screens/init.rs; please run 'cargo fmt' to format the file.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tui/screens/init.rs` around lines 1 - 4, This file has formatting
inconsistencies in the imports (e.g., the use of ratatui::{ prelude::*,
widgets::{Block, Borders, List, ListItem, ListState, Paragraph}, };) — run
`cargo fmt` to reformat the file so the import blocks and spacing conform to
rustfmt rules and CI will pass; commit the resulting changes.

Comment on lines +30 to +36
pub fn get_init_mode(&self) -> InitMode {
if self.selected == 0 {
InitMode::Default
} else {
InitMode::CustomFile(PathBuf::from(&self.path_input))
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Empty path accepted when CustomFile is selected.

When selected == 1 (CustomFile), get_init_mode() creates a PathBuf from path_input even if it's empty. This could cause issues downstream if the caller expects a valid file path.

Consider adding validation or returning an error/option when the path is empty:

Proposed validation approach
     pub fn get_init_mode(&self) -> InitMode {
         if self.selected == 0 {
             InitMode::Default
         } else {
-            InitMode::CustomFile(PathBuf::from(&self.path_input))
+            InitMode::CustomFile(PathBuf::from(self.path_input.trim()))
         }
     }

Alternatively, consider returning Option<InitMode> or validating non-empty paths at the screen transition boundary.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
pub fn get_init_mode(&self) -> InitMode {
if self.selected == 0 {
InitMode::Default
} else {
InitMode::CustomFile(PathBuf::from(&self.path_input))
}
}
pub fn get_init_mode(&self) -> InitMode {
if self.selected == 0 {
InitMode::Default
} else {
InitMode::CustomFile(PathBuf::from(self.path_input.trim()))
}
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tui/screens/init.rs` around lines 30 - 36, get_init_mode currently
constructs InitMode::CustomFile(PathBuf::from(&self.path_input)) even when
path_input is empty; change get_init_mode to return Option<InitMode> (or Result
if you prefer validation errors) and return None when selected == 1 and
self.path_input.trim().is_empty(), otherwise return
Some(InitMode::CustomFile(PathBuf::from(&self.path_input))) for the custom case
and Some(InitMode::Default) when selected == 0; update callers of get_init_mode
to handle the Option/Result accordingly.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (4)
src/tui/app.rs (3)

159-165: ⚠️ Potential issue | 🟠 Major

Keep output aligned with the selected compression.

This branch updates config.compression only. config.output still defaults to initramfs.cpio.gz, so switching to another compression silently produces a misnamed archive. Please derive the filename from Compression both in BuildConfig::default() and when leaving the compression screen.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tui/app.rs` around lines 159 - 165, The compression branch in
sync_screen_on_exit only updates config.compression, leaving config.output as
the default filename; update sync_screen_on_exit so that when
Screen::Compression is handled (where compress_screen.get_selected() is used)
you also set config.output to a filename derived from that Compression (e.g.,
use Compression to pick extension like .cpio, .cpio.gz, etc.). Also update
BuildConfig::default() so the default config.output is computed from the default
Compression value instead of hardcoding "initramfs.cpio.gz", ensuring both the
initial default and any changes via sync_screen_on_exit remain consistent with
Compression.

211-229: ⚠️ Potential issue | 🟡 Minor

Fail fast on invalid custom init scripts.

Any InitMode::CustomFile(path) is passed straight into the async build. Missing files, directories, and blank paths only surface after tokio::spawn(), which makes the wizard look like it accepted the value. Validate path.as_os_str().is_empty() / path.is_file() before switching to Screen::Build and surface a validation_error immediately.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tui/app.rs` around lines 211 - 229, Validate custom init script paths
synchronously before spawning the async build: when handling
InitMode::CustomFile(path) (the variable init_mode and enum
InitMode::CustomFile), check path.as_os_str().is_empty() and path.is_file() and
if invalid set the wizard state to show validation_error and avoid switching to
Screen::Build; only pass a proven-valid path into the
InitramfsBuilder.init_script(...) inside the tokio::spawn block so
missing/blank/non-file paths fail fast in the UI rather than during the
background build.

277-287: ⚠️ Potential issue | 🟡 Minor

Quote filesystem arguments in the exported CLI command.

--inject, --init, and -o are emitted raw here. Paths with spaces or shell metacharacters will not round-trip when users paste the command back into a shell. Quote each path-bearing argument before appending it.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tui/app.rs` around lines 277 - 287, The CLI builder appends raw
filesystem paths for injections (inj.src, inj.dest), custom init files
(InitMode::CustomFile -> path.display()), and output (self.config.output) which
breaks when paths contain spaces or shell metacharacters; update the code that
formats these flags in the loop and the init/output appends to properly quote or
shell-escape each path-bearing value (e.g., wrap in single quotes and escape
embedded single quotes, or use a shell-escaping utility) before pushing to cmd
so the exported command round-trips correctly.
src/tui/screens/init.rs (1)

30-35: ⚠️ Potential issue | 🟡 Minor

Reject blank custom init paths before returning InitMode.

This still turns "" / whitespace-only input into InitMode::CustomFile(...), so the wizard can proceed with an unusable init script and only fail later. Trim and reject the empty case here, or return Option<InitMode>/Result<InitMode, _> and make the caller block the transition.

Suggested direction
-    pub fn get_init_mode(&self) -> InitMode {
-        if self.selected == 0 {
-            InitMode::Default
-        } else {
-            InitMode::CustomFile(PathBuf::from(&self.path_input))
-        }
+    pub fn get_init_mode(&self) -> Option<InitMode> {
+        match self.selected {
+            0 => Some(InitMode::Default),
+            1 if !self.path_input.trim().is_empty() => {
+                Some(InitMode::CustomFile(PathBuf::from(self.path_input.trim())))
+            }
+            _ => None,
+        }
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tui/screens/init.rs` around lines 30 - 35, get_init_mode currently treats
whitespace/empty self.path_input as a valid InitMode::CustomFile; change its
contract to return Option<InitMode> and reject blank paths: update the signature
of get_init_mode to -> Option<InitMode>, trim self.path_input (e.g., let p =
self.path_input.trim()), return Some(InitMode::Default) when self.selected == 0,
return None when self.selected != 0 and p.is_empty(), otherwise return
Some(InitMode::CustomFile(PathBuf::from(p))); update callers to handle the None
case (block the transition or show validation error).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/tui/app.rs`:
- Around line 145-149: The enter_advanced_mode path skips syncing the active
screen, so any edits on the current Screen (e.g., Screen::Image) remain
unstored; update enter_advanced_mode to first call the same screen-exit sync
used elsewhere (call sync_screen_on_exit() or otherwise sync the current screen
state) before changing self.mode and self.screen, then call
sync_screen_on_enter() as before so config.image and other fields are up-to-date
when switching to WizardMode::Advanced.

---

Duplicate comments:
In `@src/tui/app.rs`:
- Around line 159-165: The compression branch in sync_screen_on_exit only
updates config.compression, leaving config.output as the default filename;
update sync_screen_on_exit so that when Screen::Compression is handled (where
compress_screen.get_selected() is used) you also set config.output to a filename
derived from that Compression (e.g., use Compression to pick extension like
.cpio, .cpio.gz, etc.). Also update BuildConfig::default() so the default
config.output is computed from the default Compression value instead of
hardcoding "initramfs.cpio.gz", ensuring both the initial default and any
changes via sync_screen_on_exit remain consistent with Compression.
- Around line 211-229: Validate custom init script paths synchronously before
spawning the async build: when handling InitMode::CustomFile(path) (the variable
init_mode and enum InitMode::CustomFile), check path.as_os_str().is_empty() and
path.is_file() and if invalid set the wizard state to show validation_error and
avoid switching to Screen::Build; only pass a proven-valid path into the
InitramfsBuilder.init_script(...) inside the tokio::spawn block so
missing/blank/non-file paths fail fast in the UI rather than during the
background build.
- Around line 277-287: The CLI builder appends raw filesystem paths for
injections (inj.src, inj.dest), custom init files (InitMode::CustomFile ->
path.display()), and output (self.config.output) which breaks when paths contain
spaces or shell metacharacters; update the code that formats these flags in the
loop and the init/output appends to properly quote or shell-escape each
path-bearing value (e.g., wrap in single quotes and escape embedded single
quotes, or use a shell-escaping utility) before pushing to cmd so the exported
command round-trips correctly.

In `@src/tui/screens/init.rs`:
- Around line 30-35: get_init_mode currently treats whitespace/empty
self.path_input as a valid InitMode::CustomFile; change its contract to return
Option<InitMode> and reject blank paths: update the signature of get_init_mode
to -> Option<InitMode>, trim self.path_input (e.g., let p =
self.path_input.trim()), return Some(InitMode::Default) when self.selected == 0,
return None when self.selected != 0 and p.is_empty(), otherwise return
Some(InitMode::CustomFile(PathBuf::from(p))); update callers to handle the None
case (block the transition or show validation error).

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a5ff242b-f254-4f32-948e-918fd59ce0b3

📥 Commits

Reviewing files that changed from the base of the PR and between 508b146 and 9d2b221.

📒 Files selected for processing (3)
  • src/tui/app.rs
  • src/tui/screens/init.rs
  • src/tui/ui.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/tui/ui.rs

@axelfrache axelfrache added the enhancement New feature or request label Mar 16, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
src/tui/app.rs (2)

343-362: Quote the image argument for consistency.

The image value on line 344 is not shell-quoted, but it could contain characters that need escaping (e.g., spaces in a local path, or registry URLs with special characters). For consistency with other path arguments, consider quoting it.

♻️ Proposed fix
     pub fn generate_cli_command(&self) -> String {
-        let mut cmd = format!("initramfs-builder build {}", self.config.image);
+        let mut cmd = format!("initramfs-builder build {}", shell_quote(&self.config.image));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tui/app.rs` around lines 343 - 362, The CLI builder currently inserts
self.config.image unquoted in generate_cli_command, which can break for
spaces/special chars; change the initial command construction to shell-quote the
image (use shell_quote(&self.config.image) or equivalent) so the image argument
is consistently escaped with the other path/injection arguments; update the
format call in generate_cli_command that builds "initramfs-builder build {}" to
use the quoted image string.

192-196: Consider preserving InitScreen state even when path is empty.

When get_init_mode() returns None (CustomFile selected with empty path), config.init_mode is not updated, which can cause a mismatch between UI state (init_screen.selected == 1) and config state (InitMode::Default). While validation prevents proceeding with an empty path, this silent skip could be surprising.

Consider storing the screen's selection intent even when invalid, or explicitly resetting init_screen.selected to match config.init_mode.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tui/app.rs` around lines 192 - 196, When handling Screen::Init, preserve
the InitScreen selection even when init_screen.get_init_mode() returns None:
either persist the screen's intent (e.g., record selected/index on the screen)
so config.init_mode reflects the user's selection attempt, or explicitly reset
init_screen.selected to reflect the current self.config.init_mode to avoid
UI/config mismatch; update the code around Screen::Init (where get_init_mode()
is called) to implement one of these approaches so that self.config.init_mode
and init_screen.selected remain consistent (reference InitScreen::get_init_mode,
self.config.init_mode, init_screen.selected, and InitMode::Default).
src/tui/screens/init.rs (1)

30-38: Trim whitespace from path before creating PathBuf.

The empty check uses trim() on line 33, but when creating the PathBuf on line 36, the raw path_input is used. This allows paths with leading/trailing whitespace to pass validation but potentially cause issues downstream.

♻️ Proposed fix
     pub fn get_init_mode(&self) -> Option<InitMode> {
         if self.selected == 0 {
             Some(InitMode::Default)
         } else if self.path_input.trim().is_empty() {
             None
         } else {
-            Some(InitMode::CustomFile(PathBuf::from(&self.path_input)))
+            Some(InitMode::CustomFile(PathBuf::from(self.path_input.trim())))
         }
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/tui/screens/init.rs` around lines 30 - 38, In get_init_mode, you check
the path with self.path_input.trim() but then construct the PathBuf from the raw
self.path_input, allowing leading/trailing whitespace to slip through; fix by
computing a trimmed variable (e.g. let trimmed = self.path_input.trim();) and
use that trimmed value when constructing the InitMode::CustomFile PathBuf
(PathBuf::from(trimmed)) so the validation and created PathBuf are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/tui/app.rs`:
- Around line 343-362: The CLI builder currently inserts self.config.image
unquoted in generate_cli_command, which can break for spaces/special chars;
change the initial command construction to shell-quote the image (use
shell_quote(&self.config.image) or equivalent) so the image argument is
consistently escaped with the other path/injection arguments; update the format
call in generate_cli_command that builds "initramfs-builder build {}" to use the
quoted image string.
- Around line 192-196: When handling Screen::Init, preserve the InitScreen
selection even when init_screen.get_init_mode() returns None: either persist the
screen's intent (e.g., record selected/index on the screen) so config.init_mode
reflects the user's selection attempt, or explicitly reset init_screen.selected
to reflect the current self.config.init_mode to avoid UI/config mismatch; update
the code around Screen::Init (where get_init_mode() is called) to implement one
of these approaches so that self.config.init_mode and init_screen.selected
remain consistent (reference InitScreen::get_init_mode, self.config.init_mode,
init_screen.selected, and InitMode::Default).

In `@src/tui/screens/init.rs`:
- Around line 30-38: In get_init_mode, you check the path with
self.path_input.trim() but then construct the PathBuf from the raw
self.path_input, allowing leading/trailing whitespace to slip through; fix by
computing a trimmed variable (e.g. let trimmed = self.path_input.trim();) and
use that trimmed value when constructing the InitMode::CustomFile PathBuf
(PathBuf::from(trimmed)) so the validation and created PathBuf are consistent.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2fa07376-dbbf-46ed-b9a8-af5309232cb3

📥 Commits

Reviewing files that changed from the base of the PR and between 9d2b221 and 2733994.

📒 Files selected for processing (3)
  • src/tui/app.rs
  • src/tui/events.rs
  • src/tui/screens/init.rs

@Evan-Paillard Evan-Paillard merged commit b81d9bb into main Mar 16, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants