feat: update CLI to add default tiles command#85
Conversation
📝 WalkthroughWalkthroughThe PR adds Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tiles/src/main.rs (1)
101-120: Duplication betweenNoneandRunbranches.The
Nonearm (lines 101–109) and theSome(Commands::Run { .. })arm (lines 110–120) are nearly identical — both build aRunArgsand callcommands::run. Consider extracting a small helper to reduce repetition.♻️ Proposed refactor
- None => { - // Running tiles without subcommand - launch default model with flags - let run_args = RunArgs { - modelfile_path: None, - relay_count: cli.flags.relay_count, - memory: cli.flags.memory, - }; - commands::run(&runtime, run_args).await; - } - Some(Commands::Run { - modelfile_path, - flags, - }) => { - let run_args = RunArgs { - modelfile_path, - relay_count: flags.relay_count, - memory: flags.memory, - }; - commands::run(&runtime, run_args).await; - } + None => { + let run_args = RunArgs::from_flags(None, cli.flags); + commands::run(&runtime, run_args).await; + } + Some(Commands::Run { + modelfile_path, + flags, + }) => { + let run_args = RunArgs::from_flags(modelfile_path, flags); + commands::run(&runtime, run_args).await; + }With a small helper on
RunArgs(or a free function):impl RunArgs { fn from_flags(modelfile_path: Option<String>, flags: RunFlags) -> Self { Self { modelfile_path, relay_count: flags.relay_count, memory: flags.memory, } } }
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83bdec0f59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@feynon Directing users to repl mode on Also what was the notion behind this? |
It’s inspired by recent changes in Ollama’s CLI. On first run, without any arguments, a user will typically just run the 
|
This functionally kind of does the same thing but with a different UI, but this means we need to invest some time in CLI TUI UX. And as per our plan the focus will more towards web UI. |
We should still invest in CLI/TUI I'd say. |
The reasoning for working on TUI/CLI still is we should have at least one first party in house implemented end to end client as a reference. |
Ok then closing this PR, as we will have a more focused TUI PR incoming.. |
|
Merging this since, the change you made is what we want even in modern TUI. I didn't get it completely then until i played with codex CLI today. And this kind of UX is good for doing our FTUE like account, nickname setup etc too. Like how codex asks to login to chatgpt |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tiles/src/main.rs (1)
6-15:⚠️ Potential issue | 🟠 MajorMissing
args_conflicts_with_subcommands = trueon rootCli— top-levelRunFlagsare silently accepted and discarded for all subcommands
RunFlagsis flattened ontoCli(lines 13–14) and also exists insideCommands::Run(lines 24–26). Without#[command(args_conflicts_with_subcommands = true)]onCli, clap will happily parsetiles -r 5 runortiles -m health, populatecli.flagswith those values, and then silently drop them — because allSome(Commands::…)arms never touchcli.flags. This is the exact pattern thatServerArgs,MemoryArgs, andAccountArgs(lines 69, 86, 99) all guard against by declaringargs_conflicts_with_subcommands = true.There are two ways to fix this:
Option A (preferred if the intent is that top-level flags are only for the no-subcommand path): Add
args_conflicts_with_subcommands = trueso clap rejectstiles -r 5 runwith a clear error rather than silently ignoring the flag.🛡️ Proposed fix — Option A
#[derive(Debug, Parser)] #[command(name = "tiles")] #[command(version, about = "Your private and secure AI assistant for everyday use.", long_about = None, after_help = "Documentation: https://tiles.run/book\nReport issues: https://github.com/tilesprivacy/tiles/issues")] +#[command(args_conflicts_with_subcommands = true)] struct Cli { #[command(subcommand)] command: Option<Commands>, #[command(flatten)] flags: RunFlags, }Option B (if flags should be usable with any subcommand): Mark each
RunFlagsfield with#[arg(global = true)]so they propagate down into subcommands and theRunarm reads fromcli.flagsrather than its own duplicateRunFlags.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiles/src/main.rs` around lines 6 - 15, The root Cli struct flattens RunFlags but lacks args_conflicts_with_subcommands, so top-level flags like RunFlags are silently parsed and ignored when a subcommand (Commands::Run) is present; add #[command(args_conflicts_with_subcommands = true)] to the Cli derive attributes to make clap reject invocations like `tiles -r 5 run` (preferred), or alternatively mark RunFlags fields with #[arg(global = true)] and remove the duplicate RunFlags from Commands::Run so flags propagate into subcommands (choose one approach and update the Cli and Commands::Run/RunFlags usages accordingly).
🧹 Nitpick comments (1)
tiles/src/main.rs (1)
120-139: Inconsistent error handling:commands::runsilently swallows errors while other branches propagate themBoth the
Nonearm (line 127) and theSome(Commands::Run)arm (line 138) callcommands::run, which discards the runtime error internally (let _ = runtime.run(run_args).await). The only user-visible signal is aneprintln!insidemlx.rs::run. By contrast,Some(Commands::Account)uses?(line 161) to surface errors properly. Consider refactoringcommands::runto returnResult<()>and using?here for consistency and to allow the process to exit with a non-zero status on failure.♻️ Suggested refactor
In
tiles/src/commands/mod.rs— change the signature to propagate the result:-pub async fn run(runtime: &Runtime, run_args: RunArgs) { - let _ = runtime.run(run_args).await; -} +pub async fn run(runtime: &Runtime, run_args: RunArgs) -> Result<()> { + runtime.run(run_args).await +}Then in
main.rs, both call sites become:- commands::run(&runtime, run_args).await; + commands::run(&runtime, run_args).await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tiles/src/main.rs` around lines 120 - 139, Change commands::run to return a Result (e.g., Result<(), E> or anyhow::Result<()>) and stop swallowing the error inside (propagate the error returned by runtime.run or mlx::run instead of let _ = ...). Update its signature in tiles/src/commands/mod.rs and propagate errors from any internal awaits with ? so failures surface. Then update both call sites in main.rs (the None arm and the Some(Commands::Run) arm that build a RunArgs and call commands::run(&runtime, run_args)) to use ? on commands::run so errors are propagated out of main (or convert main to return Result and use ?); ensure top-level error handling exits with non-zero status if desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tiles/src/main.rs`:
- Around line 120-128: The None branch currently jumps straight into the default
model REPL by constructing RunArgs and calling commands::run(&runtime,
run_args); instead, update this branch to present the concise launcher/overview
UI first (or route to a dedicated launcher command) before entering interactive
mode: either call a new or existing launcher function (e.g.,
commands::launcher(&runtime, launcher_args)) or emit a structured overview
(title, available models, flags summary) and only after user selection call
commands::run with the constructed RunArgs; modify the None arm to invoke that
launcher/overview flow rather than directly calling commands::run.
---
Outside diff comments:
In `@tiles/src/main.rs`:
- Around line 6-15: The root Cli struct flattens RunFlags but lacks
args_conflicts_with_subcommands, so top-level flags like RunFlags are silently
parsed and ignored when a subcommand (Commands::Run) is present; add
#[command(args_conflicts_with_subcommands = true)] to the Cli derive attributes
to make clap reject invocations like `tiles -r 5 run` (preferred), or
alternatively mark RunFlags fields with #[arg(global = true)] and remove the
duplicate RunFlags from Commands::Run so flags propagate into subcommands
(choose one approach and update the Cli and Commands::Run/RunFlags usages
accordingly).
---
Nitpick comments:
In `@tiles/src/main.rs`:
- Around line 120-139: Change commands::run to return a Result (e.g., Result<(),
E> or anyhow::Result<()>) and stop swallowing the error inside (propagate the
error returned by runtime.run or mlx::run instead of let _ = ...). Update its
signature in tiles/src/commands/mod.rs and propagate errors from any internal
awaits with ? so failures surface. Then update both call sites in main.rs (the
None arm and the Some(Commands::Run) arm that build a RunArgs and call
commands::run(&runtime, run_args)) to use ? on commands::run so errors are
propagated out of main (or convert main to return Result and use ?); ensure
top-level error handling exits with non-zero status if desired.
| None => { | ||
| // Running tiles without subcommand - launch default model with flags | ||
| let run_args = RunArgs { | ||
| modelfile_path: None, | ||
| relay_count: cli.flags.relay_count, | ||
| memory: cli.flags.memory, | ||
| }; | ||
| commands::run(&runtime, run_args).await; | ||
| } |
There was a problem hiding this comment.
None branch runs the REPL directly — diverges from the stated launcher/overview goal
The PR discussion describes showing a concise overview ("launcher") screen when tiles is invoked with no arguments, similar to Ollama's CLI. The implementation instead drops directly into the default model REPL (commands::run), which is the UX concern madclaws raised. If this branch is revisited in the follow-up TUI PR, consider emitting a structured overview before entering interactive mode, or routing to a dedicated launcher command.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tiles/src/main.rs` around lines 120 - 128, The None branch currently jumps
straight into the default model REPL by constructing RunArgs and calling
commands::run(&runtime, run_args); instead, update this branch to present the
concise launcher/overview UI first (or route to a dedicated launcher command)
before entering interactive mode: either call a new or existing launcher
function (e.g., commands::launcher(&runtime, launcher_args)) or emit a
structured overview (title, available models, flags summary) and only after user
selection call commands::run with the constructed RunArgs; modify the None arm
to invoke that launcher/overview flow rather than directly calling
commands::run.
2 participants
No description provided.