Skip to content

Commit 75015cc

Browse files
committed
refactor(cli): apply cleanup review to app-target elicitation
- exempt -v from elicitation: Vite and tsdown are cac-based and use -v, --version, so vp build -v now reaches the tool instead of the picker; the help/version predicate moves next to the other foreign-flag knowledge in help.rs - add FieldMap::contains for presence checks without cloning the field value; use it in the pack-runnable rule - store PackageRow name/path as Str, deleting the Str->String->Str round-trip; flatten the picker branch and the AppTarget dispatch into one exhaustive match; inline the one-line should_enable_picker wrapper; test and comment tidying
1 parent 0001748 commit 75015cc

5 files changed

Lines changed: 47 additions & 34 deletions

File tree

crates/vite_global_cli/src/command_picker.rs

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ const COMMANDS: &[CommandEntry] = &[
117117
pub fn pick_top_level_command_if_interactive(
118118
cwd: &AbsolutePath,
119119
) -> io::Result<TopLevelCommandPick> {
120-
if !should_enable_picker() {
120+
if !vite_shared::is_interactive_terminal() {
121121
return Ok(TopLevelCommandPick::Skipped);
122122
}
123123

@@ -129,10 +129,6 @@ pub fn pick_top_level_command_if_interactive(
129129
})
130130
}
131131

132-
fn should_enable_picker() -> bool {
133-
vite_shared::is_interactive_terminal()
134-
}
135-
136132
fn run_picker(command_order: &[usize]) -> io::Result<Option<PickedCommand>> {
137133
let mut stdout = io::stdout();
138134
let mut selected_position = 0usize;

crates/vite_static_config/src/lib.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,17 @@ impl FieldMap {
6969
}
7070
}
7171
}
72+
73+
/// Presence check with the same semantics as [`get`](Self::get) returning
74+
/// `Some`, without cloning the field's value. In an open map (spread or
75+
/// computed keys) every key may exist and therefore counts as present.
76+
#[must_use]
77+
pub fn contains(&self, key: &str) -> bool {
78+
match &self.0 {
79+
FieldMapInner::Closed(map) => map.contains_key(key),
80+
FieldMapInner::Open(_) => true,
81+
}
82+
}
7283
}
7384

7485
/// Config file names to try, in priority order.

packages/cli/binding/src/cli/app_target.rs

Lines changed: 26 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,8 @@ pub(super) enum AppTarget {
2626
}
2727

2828
struct PackageRow {
29-
name: String,
30-
path: String,
29+
name: vite_str::Str,
30+
path: vite_str::Str,
3131
absolute: AbsolutePathBuf,
3232
runnable: bool,
3333
}
@@ -49,9 +49,8 @@ fn app_command_parts(subcommand: &SynthesizableSubcommand) -> Option<(&'static s
4949
/// disables elicitation; help/version requests are answered by the underlying
5050
/// tool and must never be redirected.
5151
fn is_bare(args: &[String]) -> bool {
52-
args.iter().all(|arg| {
53-
arg.starts_with('-') && !matches!(arg.as_str(), "-h" | "--help" | "-V" | "--version")
54-
})
52+
args.iter()
53+
.all(|arg| arg.starts_with('-') && !super::help::is_app_tool_help_or_version_flag(arg))
5554
}
5655

5756
/// Heuristic ranking signal: does `dir` look runnable for `command`?
@@ -62,9 +61,11 @@ fn looks_runnable(dir: &AbsolutePathBuf, command: &str) -> bool {
6261
match command {
6362
// Bare `vp pack` succeeds when the config declares a `pack` block or
6463
// tsdown's default entry exists; a Vite config without `pack` does
65-
// not make a package packable.
64+
// not make a package packable. `contains` deliberately counts configs
65+
// with spreads (`pack` may exist behind them): the right direction
66+
// for a never-hide ranking signal.
6667
"pack" => {
67-
vite_static_config::resolve_static_config(dir).get("pack").is_some()
68+
vite_static_config::resolve_static_config(dir).contains("pack")
6869
|| dir.as_path().join("src/index.ts").is_file()
6970
}
7071
_ => vite_static_config::has_config_file(dir) || dir.as_path().join("index.html").is_file(),
@@ -107,8 +108,8 @@ fn run_package_picker(command: &str, rows: &[PackageRow]) -> Result<Option<usize
107108
.iter()
108109
.map(|row| vite_select::SelectItem {
109110
label: vite_str::format!("{} {}", row.name, row.path),
110-
display_name: vite_str::Str::from(row.name.as_str()),
111-
description: vite_str::Str::from(row.path.as_str()),
111+
display_name: row.name.clone(),
112+
description: row.path.clone(),
112113
group: None,
113114
})
114115
.collect();
@@ -181,8 +182,8 @@ pub(super) fn resolve_app_target(
181182
.map(|info| {
182183
let absolute = info.absolute_path.to_absolute_path_buf();
183184
PackageRow {
184-
name: info.package_json.name.to_string(),
185-
path: info.path.as_str().to_string(),
185+
name: info.package_json.name.clone(),
186+
path: vite_str::Str::from(info.path.as_str()),
186187
runnable: looks_runnable(&absolute, command),
187188
absolute,
188189
}
@@ -198,14 +199,13 @@ pub(super) fn resolve_app_target(
198199
// otherwise the fuzzy picker runs.
199200
if vite_shared::is_interactive_terminal() {
200201
let single_runnable = rows[0].runnable && rows.get(1).is_none_or(|row| !row.runnable);
201-
let row = if single_runnable {
202-
&rows[0]
203-
} else {
204-
match run_package_picker(command, &rows)? {
205-
Some(index) => &rows[index],
206-
None => return Ok(AppTarget::Exit(ExitStatus(130))),
207-
}
202+
let picked = if single_runnable { Some(0) } else { run_package_picker(command, &rows)? };
203+
let Some(index) = picked else {
204+
return Ok(AppTarget::Exit(ExitStatus(130)));
208205
};
206+
let row = &rows[index];
207+
// Deliberately stdout via println!: these lines belong to the
208+
// command's own output stream, like the tool output that follows.
209209
println!("Selected package: {} ({})", row.name, row.path);
210210
println!("Tip: run this directly with `vp -C {} {command}`", row.path);
211211
return Ok(AppTarget::Dir(row.absolute.clone()));
@@ -244,18 +244,19 @@ mod tests {
244244
assert!(!is_bare(&to_args(&["--help"])));
245245
assert!(!is_bare(&to_args(&["-h"])));
246246
assert!(!is_bare(&to_args(&["--watch", "--version"])));
247+
// Vite and tsdown are cac-based and use `-v` for version.
248+
assert!(!is_bare(&to_args(&["-v"])));
247249
}
248250

249251
#[test]
250252
fn only_app_commands_elicit() {
251-
let args = vec![];
252253
for (subcommand, expected) in [
253-
(SynthesizableSubcommand::Dev { args: args.clone() }, Some("dev")),
254-
(SynthesizableSubcommand::Build { args: args.clone() }, Some("build")),
255-
(SynthesizableSubcommand::Preview { args: args.clone() }, Some("preview")),
256-
(SynthesizableSubcommand::Pack { args: args.clone() }, Some("pack")),
257-
(SynthesizableSubcommand::Lint { args: args.clone() }, None),
258-
(SynthesizableSubcommand::Test { args: args.clone() }, None),
254+
(SynthesizableSubcommand::Dev { args: vec![] }, Some("dev")),
255+
(SynthesizableSubcommand::Build { args: vec![] }, Some("build")),
256+
(SynthesizableSubcommand::Preview { args: vec![] }, Some("preview")),
257+
(SynthesizableSubcommand::Pack { args: vec![] }, Some("pack")),
258+
(SynthesizableSubcommand::Lint { args: vec![] }, None),
259+
(SynthesizableSubcommand::Test { args: vec![] }, None),
259260
] {
260261
assert_eq!(app_command_parts(&subcommand).map(|(name, _)| name), expected);
261262
}

packages/cli/binding/src/cli/help.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,13 @@ fn is_vitest_help_flag(arg: &str) -> bool {
3636
matches!(arg, "-h" | "--help")
3737
}
3838

39+
/// Help/version flags of the forwarded app tools (Vite and tsdown are
40+
/// cac-based: `-v, --version`; vp's own clap surface uses `-V`). Requests for
41+
/// these must always reach the tool, never target elicitation.
42+
pub(super) fn is_app_tool_help_or_version_flag(arg: &str) -> bool {
43+
matches!(arg, "-h" | "--help" | "-v" | "-V" | "--version")
44+
}
45+
3946
fn is_vitest_watch_flag(arg: &str) -> bool {
4047
matches!(arg, "-w" | "--watch")
4148
}

packages/cli/binding/src/cli/mod.rs

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,12 +51,10 @@ async fn execute_direct_subcommand(
5151
// (defaultPackage, package listing); the command then runs as if invoked
5252
// in the resolved directory (rfcs/cwd-flag.md).
5353
let target = app_target::resolve_app_target(&subcommand, cwd)?;
54-
if let app_target::AppTarget::Exit(status) = target {
55-
return Ok(status);
56-
}
5754
let cwd = match &target {
55+
app_target::AppTarget::Exit(status) => return Ok(*status),
5856
app_target::AppTarget::Dir(dir) => dir,
59-
_ => cwd,
57+
app_target::AppTarget::CurrentDir => cwd,
6058
};
6159

6260
let (workspace_root, _) = vite_workspace::find_workspace_root(cwd)?;

0 commit comments

Comments
 (0)