Conversation
…nd APIs to connect to the CLI\n Signed-off-by: rafaeljohn9 <rafaeljohb@gmail.com>
Signed-off-by: rafaeljohn9 <rafaeljohb@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
This PR implements a Cargo Tracker CLI application, introducing modules for managing shipments and packages.
- Adds a ShipmentManager to create, update, and list shipments.
- Introduces the Shipment and Package models with JSON serialization.
- Develops a CLI interface in main.rs and updates Cargo.toml with new dependencies.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cargo-tracker/src/shipment_manager.rs | Implements shipment management features with HashMap storage. |
| cargo-tracker/src/shipment.rs | Defines data structures (Shipment, Package) and JSON serialization logic. |
| cargo-tracker/src/main.rs | Creates the CLI commands for user interaction and shipment operations. |
| cargo-tracker/Cargo.toml | Includes new dependencies (chrono, uuid, serde_json) required for functionality. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughTwo new Rust projects are introduced: a basic Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Main CLI Loop
participant Manager as ShipmentManager
participant Shipment
participant Package
User->>CLI: Enter command
CLI->>CLI: Parse command
alt add-shipment
CLI->>Manager: create_shipment()
Manager->>Shipment: new()
Shipment-->>Manager: Shipment instance
Manager-->>CLI: &mut Shipment
else add-package
CLI->>Manager: get_shipment(tracking_id)
Manager-->>CLI: &mut Shipment
CLI->>Package: new(description)
Package-->>CLI: Package instance
CLI->>Shipment: add_package()
else update-status
CLI->>Manager: get_shipment(tracking_id)
Manager-->>CLI: &mut Shipment
alt status == Delivered
Shipment->>Shipment: time_of_arrival = now()
end
else list-shipments
CLI->>Manager: list_shipments(optional_filter)
Manager-->>CLI: Vec<&Shipment>
CLI->>CLI: Format and display
end
CLI-->>User: Display result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The diff spans multiple new modules with heterogeneous purposes (CLI, models, manager), featuring moderate logic density in command parsing and status transitions, but with repetitive patterns within each component and straightforward data structure definitions that somewhat offset complexity. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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.
Actionable comments posted: 3
♻️ Duplicate comments (2)
cargo-tracker/src/main.rs (2)
53-61: Previous “time_of_arrival passed as departure” issue looks resolved.Now using
let time_of_departure = Some(chrono::Utc::now());and passing it correctly. No action needed.
96-100: Unintended app exit on empty package description. Replace return with continue.Returning here terminates the whole CLI loop.
- if description.trim().is_empty() { - println!("Package description cannot be empty."); - return; - } + if description.trim().is_empty() { + println!("Package description cannot be empty."); + continue; + }
🧹 Nitpick comments (11)
cargo-tracker/src/main.rs (4)
63-77: Guard against empty package descriptions when adding to a new shipment.Mirror the validation used in add-package to avoid empty entries.
- let package = Package::new(description); - shipment.add_package(package); - count += 1; - package_num += 1; + if description.trim().is_empty() { + println!("Package description cannot be empty."); + } else { + let package = Package::new(description); + shipment.add_package(package); + count += 1; + package_num += 1; + }
166-177: Clarify invalid status filter message and behavior.Currently an invalid filter prints “No shipments found…”, which is misleading. Prefer “Invalid status; showing all.”
- _ => { - println!("No shipments found for status '{}'.", filter); - None - } + _ => { + println!( + "Invalid status filter '{}'. Showing all shipments. Valid: Pending, InTransit, Delivered, Lost.", + filter.trim() + ); + None + }
207-209: Flush after clearing screen (minor).Ensure the ANSI clear takes effect immediately.
- print!("\x1B[2J\x1B[1;1H"); + print!("\x1B[2J\x1B[1;1H"); + io::stdout().flush().ok();
210-213: Usebreakinstead ofprocess::exit(0)to enable proper resource cleanup.The REPL loop (line 36) can exit cleanly with
break, allowing Drop implementations to run. The codebase already usesbreakfor similar patterns (line 71). Usingexit(0)skips cleanup and is unnecessary here.- println!("Goodbye!"); - exit(0); + println!("Goodbye!"); + break;cargo-tracker/src/shipment_manager.rs (2)
15-26: Avoid silent overwrite on duplicate tracking IDs; return a Result.If
tracking_idalready exists,insertreplaces the shipment. Consider failing fast.- ) -> &mut Shipment { - let id = tracking_id.unwrap_or_else(|| uuid::Uuid::new_v4().to_string()); - let shipment = Shipment::new(status, destination, time_of_departure, id.clone()); - self.shipments.insert(id.clone(), shipment); - self.shipments.get_mut(&id).unwrap() + ) -> Result<&mut Shipment, String> { + let id = tracking_id.unwrap_or_else(|| uuid::Uuid::new_v4().to_string()); + if self.shipments.contains_key(&id) { + return Err(format!("Shipment with tracking ID '{}' already exists.", id)); + } + let shipment = Shipment::new(status, destination, time_of_departure, id.clone()); + self.shipments.insert(id.clone(), shipment); + Ok(self.shipments.get_mut(&id).expect("inserted above"))Follow-up: update call sites to handle
Resultand print the error.
28-30: Split read-only and mutable getters for ergonomic borrows.Requiring
&mut selfeven for reads makes CLI code borrow mutably when unnecessary and limits parallel reads.- pub fn get_shipment(&mut self, tracking_id: &str) -> Option<&mut Shipment> { - self.shipments.get_mut(tracking_id) - } + pub fn get_shipment(&self, tracking_id: &str) -> Option<&Shipment> { + self.shipments.get(tracking_id) + } + + pub fn get_shipment_mut(&mut self, tracking_id: &str) -> Option<&mut Shipment> { + self.shipments.get_mut(tracking_id) + }Additional changes:
- In main.rs where you only read (existence checks, view), switch to
get_shipment.- Where you mutate (add-package, update-status), use
get_shipment_mut.I can push the corresponding main.rs diff if you want.
cargo-tracker/Cargo.toml (1)
6-11: Move test-only crates to [dev-dependencies].
assert_cmdandpredicatesare used only in test files and should not be included in release builds.-[dependencies] -assert_cmd = "2" -predicates = "2" -chrono = "0.4" -uuid = { version = "1", features = ["v4"] } -serde_json = "1" +[dependencies] +chrono = "0.4" +uuid = { version = "1", features = ["v4"] } +serde_json = "1" + +[dev-dependencies] +assert_cmd = "2" +predicates = "2"cargo-tracker/src/shipment.rs (4)
1-3: Consider using serde for serialization instead of manual JSON construction.The imports currently support manual JSON building via
serde_json::json!. For better maintainability and type safety, consider using serde'sSerializeandDeserializederives instead.Add serde to your imports:
use chrono::{DateTime, Utc}; -use serde_json::json; +use serde::{Deserialize, Serialize}; use uuid::Uuid;Then derive
Serializeon your types (see subsequent comments for full refactor).
5-13: Consider additional derives for Shipment.The
Shipmentstruct only derivesDebug, but addingClonewould be beneficial for common operations like storing in collections or passing around. While all fields are currently public, consider whether encapsulation would be appropriate for your use case.-#[derive(Debug)] +#[derive(Debug, Clone)] pub struct Shipment {
16-30: Consider adding input validation.The constructor accepts
tracking_idanddestinationwithout validation. Empty strings or whitespace-only values could lead to invalid shipments.pub fn new( status: ShipmentStatus, destination: String, time_of_departure: Option<DateTime<Utc>>, tracking_id: String, ) -> Self { + if tracking_id.trim().is_empty() { + panic!("tracking_id cannot be empty"); + } + if destination.trim().is_empty() { + panic!("destination cannot be empty"); + } Self { packages: Vec::new(), tracking_id, destination, status, time_of_departure, time_of_arrival: None, } }Alternatively, return a
Result<Self, String>for more graceful error handling.
69-73: Consider adding PartialEq derive.The
Packagestruct is well-structured. AddingPartialEqwould enable equality comparisons, which can be useful for testing.-#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq)] pub struct Package {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
bit-torrent/Cargo.toml(1 hunks)bit-torrent/src/main.rs(1 hunks)cargo-tracker/Cargo.toml(1 hunks)cargo-tracker/src/main.rs(1 hunks)cargo-tracker/src/shipment.rs(1 hunks)cargo-tracker/src/shipment_manager.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
cargo-tracker/src/shipment_manager.rs (1)
cargo-tracker/src/shipment.rs (2)
new(16-30)new(76-81)
cargo-tracker/src/main.rs (2)
cargo-tracker/src/shipment.rs (3)
new(16-30)new(76-81)packages(37-46)cargo-tracker/src/shipment_manager.rs (1)
new(9-13)
cargo-tracker/src/shipment.rs (1)
cargo-tracker/src/shipment_manager.rs (1)
new(9-13)
🔇 Additional comments (5)
bit-torrent/Cargo.toml (2)
4-4: Verify CI and local toolchain support Rust 1.85 or later.Rust 2024 edition requires Rust 1.85 or newer. All three crates in this repository already use
edition = "2024", and Rust 1.85 was released with edition 2024. Ensure your CI configuration and developer toolchains meet this minimum version requirement, or builds will fail.
1-6: No workspace exists; bit-torrent is a standalone package.The repository has no root
Cargo.tomlwith[workspace]configuration. The three packages (bit-torrent,cargo-tracker,ratatui) are independent sibling directories, not workspace members. The workspace membership check in the original comment is not applicable here.Likely an incorrect or invalid review comment.
cargo-tracker/src/shipment.rs (3)
32-34: LGTM!The
add_packagemethod is straightforward and correct.
61-67: LGTM!The
ShipmentStatusenum variants are well-chosen and appropriately cover the shipment lifecycle. The derives (Debug,Clone,PartialEq) are suitable for this enum.
75-81: LGTM!The
Package::newconstructor correctly generates a unique UUID v4 for each package instance.
bit-torrent/src/main.rs
Outdated
| for i in 1..10{ | ||
| println!("i: {}",i); | ||
| } |
There was a problem hiding this comment.
Range excludes 10 (minor off-by-one).
1..10 iterates 1..=9. If you intended to include 10, use an inclusive range.
- for i in 1..10{
+ for i in 1..=10 {
println!("i: {}",i);
}📝 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.
| for i in 1..10{ | |
| println!("i: {}",i); | |
| } | |
| for i in 1..=10 { | |
| println!("i: {}",i); | |
| } |
🤖 Prompt for AI Agents
In bit-torrent/src/main.rs around lines 4 to 6, the loop uses an exclusive range
1..10 which iterates 1 through 9; to include 10 change the range to an inclusive
range 1..=10 (or if exclusion was intended, leave as-is); update the range
expression accordingly so the loop covers the intended bounds.
| _ => { | ||
| println!( | ||
| "Error: Invalid status. Valid options are: Pending, InTransit, Delivered, Lost." | ||
| ); | ||
| return; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Do not exit the CLI on invalid status; continue the loop.
return ends the program. Stay in REPL and prompt again.
- _ => {
- println!(
- "Error: Invalid status. Valid options are: Pending, InTransit, Delivered, Lost."
- );
- return;
- }
+ _ => {
+ println!(
+ "Error: Invalid status. Valid options are: Pending, InTransit, Delivered, Lost."
+ );
+ continue;
+ }📝 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.
| _ => { | |
| println!( | |
| "Error: Invalid status. Valid options are: Pending, InTransit, Delivered, Lost." | |
| ); | |
| return; | |
| } | |
| }; | |
| _ => { | |
| println!( | |
| "Error: Invalid status. Valid options are: Pending, InTransit, Delivered, Lost." | |
| ); | |
| continue; | |
| } | |
| }; |
🤖 Prompt for AI Agents
In cargo-tracker/src/main.rs around lines 124 to 130 the code prints an error on
invalid status and then calls return which exits the CLI; instead remove the
return and continue the input loop so the REPL keeps running. Replace the early
return with a continue (or otherwise loop-continue control flow) so after
printing the error the program re-prompts the user rather than terminating.
| pub fn to_json_str(&self) -> String { | ||
| let packages_json = self | ||
| .packages | ||
| .iter() | ||
| .map(|p| { | ||
| json!({ | ||
| "id": p.id.to_string(), | ||
| "description": p.description, | ||
| }) | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let json_obj = json!({ | ||
| "tracking_id": self.tracking_id, | ||
| "destination": self.destination, | ||
| "status": format!("{:?}", self.status), | ||
| "time_of_departure": self.time_of_departure.map(|t| t.to_rfc3339()), | ||
| "time_of_arrival": self.time_of_arrival.map(|t| t.to_rfc3339()), | ||
| "packages": packages_json, | ||
| }); | ||
|
|
||
| json_obj.to_string() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor to use serde's Serialize derive instead of manual JSON construction.
Manually building JSON is error-prone and harder to maintain. Since serde_json is already a dependency, leverage serde's Serialize derive for type-safe, automatic serialization. This also addresses the improper use of Debug format for the status enum on line 51.
Add serde to Cargo.toml if not already present with the derive feature:
serde = { version = "1", features = ["derive"] }Then refactor the structs:
+use serde::{Deserialize, Serialize};
use chrono::{DateTime, Utc};
-use serde_json::json;
use uuid::Uuid;
-#[derive(Debug)]
+#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct Shipment {
pub tracking_id: String,
pub destination: String,
pub packages: Vec<Package>,
pub status: ShipmentStatus,
pub time_of_departure: Option<DateTime<Utc>>,
pub time_of_arrival: Option<DateTime<Utc>>,
}-#[derive(Debug, Clone, PartialEq)]
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
pub enum ShipmentStatus {
Pending,
InTransit,
Delivered,
Lost,
}-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct Package {
pub id: Uuid,
pub description: String,
}Replace the entire to_json_str method:
pub fn to_json_str(&self) -> String {
- let packages_json = self
- .packages
- .iter()
- .map(|p| {
- json!({
- "id": p.id.to_string(),
- "description": p.description,
- })
- })
- .collect::<Vec<_>>();
-
- let json_obj = json!({
- "tracking_id": self.tracking_id,
- "destination": self.destination,
- "status": format!("{:?}", self.status),
- "time_of_departure": self.time_of_departure.map(|t| t.to_rfc3339()),
- "time_of_arrival": self.time_of_arrival.map(|t| t.to_rfc3339()),
- "packages": packages_json,
- });
-
- json_obj.to_string()
+ serde_json::to_string(self).unwrap_or_else(|_| String::from("{}"))
}
RafaelJohn9
force-pushed
the
rafaeljohn9/cargo-tracker
branch
from
a13cac8 to
3714969
Compare
🚀 Shipment On Schedule! cargo-tracker Tests Passed!📦 All packages inspected and cleared for delivery. ✅ The cargo is stable, the manifest is clean, and we’re on track for a smooth merge!
|
Summary by CodeRabbit
New Features
Chores