open responses and mem model compatibility + fixes#83
Conversation
📝 WalkthroughWalkthroughThis pull request bumps the package version from 0.4.0-rc.1 to 0.4.0 across the installation script and Rust workspace. Dependencies are updated (numpy 2.4.1→2.4.2, tqdm 4.67.1→4.67.3) with corresponding lock files refreshed. A pluggable Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tiles/src/runtime/mlx.rs`:
- Around line 491-496: The HTTP POST currently uses
client.post(api_url).json(&body).send().await.unwrap(), which will panic on
failures; change this to propagate errors into the function's Result by
replacing the unwrap with error mapping/propagation (e.g., await the send and
use ? or map_err to convert reqwest errors into the function's String error
type), and likewise handle non-success HTTP statuses before parsing the response
into ChatResponse; update the variables referenced (api_url, run_args.memory,
and res) so send errors and bad-status responses return Err(String) instead of
panicking.
- Around line 206-214: The show_help() function currently calls
get_memory_path().unwrap(), which can panic; change it to handle the Result:
call get_memory_path() and if Ok(path) print it, otherwise call
get_default_memory_path() and use that when available, or print "<not
configured>" as the fallback; update the print block in show_help() to use this
non-panicking value so the REPL no longer crashes when memory path is missing.
🧹 Nitpick comments (2)
tiles/src/utils/config.rs (1)
19-28: Replace deprecatedstd::env::home_dirfor home resolution.
std::env::home_diris deprecated; consider a maintained helper (e.g.,dirs::home_dir/homecrate) or a platform-aware env var fallback to avoid warnings and future breakage.
As per coding guidelines, **/*.{rs,toml}: Review the Rust code for conformity with best practices in Rust, Systems programming. Highlight any deviations.Also applies to: 33-43, 51-53
tiles/tests/config.rs (1)
22-65: Commented-out integration tests will never run.Either enable these tests (and expose provider-based helpers for integration tests) or remove the commented blocks to avoid dead code.
As per coding guidelines, **/*.{rs,toml}: Review the Rust code for conformity with best practices in Rust, Systems programming. Highlight any deviations.
| fn show_help(model_name: &str) { | ||
| println!("\n=== Tiles REPL Help ===\n"); | ||
| println!("\n=== Tiles REPL ===\n"); | ||
|
|
||
| println!("Model:"); | ||
| println!(" {}", model_name); | ||
|
|
||
| println!("Directory:"); | ||
| println!(" {}\n", get_memory_path().unwrap()); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the repository structure
git ls-files | head -20Repository: tilesprivacy/tiles
Length of output: 351
🏁 Script executed:
# Check if the file exists
ls -la tiles/src/runtime/mlx.rsRepository: tilesprivacy/tiles
Length of output: 136
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '206,214p' tiles/src/runtime/mlx.rsRepository: tilesprivacy/tiles
Length of output: 275
🏁 Script executed:
# Let's see more context around those lines
sed -n '200,220p' tiles/src/runtime/mlx.rsRepository: tilesprivacy/tiles
Length of output: 551
🏁 Script executed:
# Find the get_memory_path function definition
rg "fn get_memory_path" -A 5Repository: tilesprivacy/tiles
Length of output: 544
🏁 Script executed:
# Also search for get_default_memory_path to see if it exists
rg "get_default_memory_path"Repository: tilesprivacy/tiles
Length of output: 522
🏁 Script executed:
# Search for all calls to get_memory_path to understand usage patterns
rg "get_memory_path\(\)" -B 2 -A 2Repository: tilesprivacy/tiles
Length of output: 1085
🏁 Script executed:
# Get the full implementation of get_memory_path
sed -n '/^pub fn get_memory_path/,/^}/p' tiles/src/utils/config.rs | head -30Repository: tilesprivacy/tiles
Length of output: 593
🏁 Script executed:
# Get the full implementation of get_default_memory_path
sed -n '/^pub fn get_default_memory_path/,/^}/p' tiles/src/utils/config.rs | head -30Repository: tilesprivacy/tiles
Length of output: 248
🏁 Script executed:
# Check get_or_set_memory_path to see how it handles errors
sed -n '/^fn get_or_set_memory_path/,/^}/p' tiles/src/runtime/mlx.rs | head -40Repository: tilesprivacy/tiles
Length of output: 1814
🏁 Script executed:
# Check what Result type is being used (custom Result or std::Result)
rg "^type Result" tiles/src/utils/config.rsRepository: tilesprivacy/tiles
Length of output: 44
🏁 Script executed:
# Look at the beginning of config.rs to see what Result is
head -30 tiles/src/utils/config.rsRepository: tilesprivacy/tiles
Length of output: 1018
Handle error gracefully in show_help() instead of panicking.
get_memory_path() returns Result<String> and can fail when memory path isn't configured, causing unwrap() to panic the REPL. Replace with error handling using get_default_memory_path() as a fallback, or display "" instead.
🛠️ Proposed fix
- println!(" {}\n", get_memory_path().unwrap());
+ let mem_path = match get_memory_path() {
+ Ok(p) => p,
+ Err(_) => get_default_memory_path()
+ .map(|p| p.display().to_string())
+ .unwrap_or_else(|_| "<not configured>".to_string()),
+ };
+ println!(" {}\n", mem_path);🤖 Prompt for AI Agents
In `@tiles/src/runtime/mlx.rs` around lines 206 - 214, The show_help() function
currently calls get_memory_path().unwrap(), which can panic; change it to handle
the Result: call get_memory_path() and if Ok(path) print it, otherwise call
get_default_memory_path() and use that when available, or print "<not
configured>" as the fallback; update the print block in show_help() to use this
non-panicking value so the REPL no longer crashes when memory path is missing.
| let api_url = if run_args.memory { | ||
| "http://127.0.0.1:6969/v1/chat/completions" | ||
| } else { | ||
| "http://127.0.0.1:6969/v1/responses" | ||
| }; | ||
| let res = client.post(api_url).json(&body).send().await.unwrap(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd tiles && find . -name "mlx.rs" -type fRepository: tilesprivacy/tiles
Length of output: 82
🏁 Script executed:
wc -l tiles/src/runtime/mlx.rsRepository: tilesprivacy/tiles
Length of output: 90
🏁 Script executed:
sed -n '480,510p' tiles/src/runtime/mlx.rsRepository: tilesprivacy/tiles
Length of output: 1077
🏁 Script executed:
sed -n '470,490p' tiles/src/runtime/mlx.rsRepository: tilesprivacy/tiles
Length of output: 559
🏁 Script executed:
sed -n '500,530p' tiles/src/runtime/mlx.rsRepository: tilesprivacy/tiles
Length of output: 1196
🏁 Script executed:
sed -n '491,496p' tiles/src/runtime/mlx.rsRepository: tilesprivacy/tiles
Length of output: 287
Propagate HTTP request errors instead of panicking with unwrap().
The client.post(api_url).send().await.unwrap() call will panic on network failures. Since the function returns Result<ChatResponse, String>, errors should be propagated to allow the REPL to recover.
🛠️ Suggested fix
- let res = client.post(api_url).json(&body).send().await.unwrap();
+ let res = client
+ .post(api_url)
+ .json(&body)
+ .send()
+ .await
+ .map_err(|e| e.to_string())?;📝 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.
| let api_url = if run_args.memory { | |
| "http://127.0.0.1:6969/v1/chat/completions" | |
| } else { | |
| "http://127.0.0.1:6969/v1/responses" | |
| }; | |
| let res = client.post(api_url).json(&body).send().await.unwrap(); | |
| let api_url = if run_args.memory { | |
| "http://127.0.0.1:6969/v1/chat/completions" | |
| } else { | |
| "http://127.0.0.1:6969/v1/responses" | |
| }; | |
| let res = client | |
| .post(api_url) | |
| .json(&body) | |
| .send() | |
| .await | |
| .map_err(|e| e.to_string())?; |
🤖 Prompt for AI Agents
In `@tiles/src/runtime/mlx.rs` around lines 491 - 496, The HTTP POST currently
uses client.post(api_url).json(&body).send().await.unwrap(), which will panic on
failures; change this to propagate errors into the function's Result by
replacing the unwrap with error mapping/propagation (e.g., await the send and
use ? or map_err to convert reqwest errors into the function's String error
type), and likewise handle non-success HTTP statuses before parsing the response
into ChatResponse; update the variables referenced (api_url, run_args.memory,
and res) so send errors and bad-status responses return Err(String) instead of
panicking.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
No description provided.