-
-
Notifications
You must be signed in to change notification settings - Fork 169
feat (cli): added payment handling along with cloud instance creation to cli, also added better sse stream handling and modified login #730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (4)
-
helix-cli/src/commands/add.rs, line 192-194 (link)logic: Duplicate config save - this is already done on line 82 for Helix instances
-
helix-cli/src/lib.rs, line 45-47 (link)logic: Region parameter uses Option<String> but has default_value - this creates confusing behavior where the Option will never be None
Should this be a plain String since it always has a default value, or should the default_value be removed?
-
helix-cli/src/commands/auth.rs, line 44 (link)logic: Using
.unwrap()here will cause the program to panic if login fails. Consider propagating the error instead with?operator for better error handling. -
helix-cli/src/commands/auth.rs, line 55 (link)logic: Memory leak:
user_id.leak()creates a memory leak by converting the String to a static str. Consider using a different approach to satisfy the lifetime requirements without leaking memory.
17 files reviewed, 9 comments
|
|
||
| /// Update progress percentage | ||
| pub fn set_progress(&self, percentage: f64) { | ||
| self.progress_bar.set_position(percentage as u64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Progress percentage cast from f64 to u64 could lose precision and truncate decimal values. Should fractional progress percentages be preserved or is integer precision sufficient?
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-cli/src/sse_client.rs
Line: 188:188
Comment:
**logic:** Progress percentage cast from f64 to u64 could lose precision and truncate decimal values. Should fractional progress percentages be preserved or is integer precision sufficient?
How can I resolve this? If you propose a fix, please make it concise.| // Create cloud instance configuration (without cluster_id yet) | ||
| let cloud_config = helix_manager | ||
| .create_instance_config(project_name, region) | ||
| .create_instance_config(project_name, region.clone()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Cloning region here is unnecessary since it's used again on line 142 - consider using a reference or moving the clone.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-cli/src/commands/init.rs
Line: 111:111
Comment:
**style:** Cloning region here is unnecessary since it's used again on line 142 - consider using a reference or moving the clone.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| // The server will send CheckoutRequired, PaymentConfirmed, CreatingProject, ProjectCreated events | ||
| print_status("INITIATING", "Starting cluster creation..."); | ||
|
|
||
| let create_url = format!("http://{}/create-cluster", *CLOUD_AUTHORITY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Using HTTP instead of HTTPS for production cloud service communication is a security risk
| let create_url = format!("http://{}/create-cluster", *CLOUD_AUTHORITY); | |
| let create_url = format!("https://{}/create-cluster", *CLOUD_AUTHORITY); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-cli/src/commands/create_cluster.rs
Line: 60:60
Comment:
**logic:** Using HTTP instead of HTTPS for production cloud service communication is a security risk
```suggestion
let create_url = format!("https://{}/create-cluster", *CLOUD_AUTHORITY);
```
How can I resolve this? If you propose a fix, please make it concise.| let mut event_source = client | ||
| .post(&create_url) | ||
| .header("x-api-key", &credentials.helix_admin_key) | ||
| .header("Content-Type", "application/json") | ||
| .eventsource()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: POST request has Content-Type: application/json but no request body is being sent. Should the request include cluster parameters like instance_name and region in the body?
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-cli/src/commands/create_cluster.rs
Line: 64:68
Comment:
**logic:** POST request has Content-Type: application/json but no request body is being sent. Should the request include cluster parameters like instance_name and region in the body?
How can I resolve this? If you propose a fix, please make it concise.| } | ||
| Ok(reqwest_eventsource::Event::Message(message)) => { | ||
| // Debug: print raw data | ||
| eprintln!("DEBUG: Received SSE data: {}", message.data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Debug output using eprintln! should be removed or replaced with proper logging for production code
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-cli/src/commands/create_cluster.rs
Line: 81:81
Comment:
**style:** Debug output using `eprintln!` should be removed or replaced with proper logging for production code
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
NOTE: the cloud authority url is included, should probably be made into a GitHub env var on build.
Greptile Overview
Greptile Summary
This PR implements payment handling and cloud instance creation functionality for the Helix CLI, along with improved SSE (Server-Sent Events) stream handling and login modifications. The changes introduce a new
create-clustercommand that enables users to provision managed database clusters through a Stripe-integrated payment workflow. Key architectural improvements include refactoring authentication from WebSocket to SSE, adding real-time deployment progress tracking, and restructuring cloud configurations to support multiple environments and providers.The implementation follows a two-phase approach: first creating instance configurations, then prompting users for cluster creation with clear payment implications. This gives users explicit control over when they incur charges while preserving configurations if they defer cluster creation.
PR Description Notes:
Important Files Changed
helix-cli/src/commands/create_cluster.rshelix-cli/src/sse_client.rshelix-cli/src/commands/integrations/helix.rshelix-cli/src/commands/add.rshelix-cli/src/commands/push.rshelix-cli/src/commands/init.rshelix-cli/src/commands/auth.rshelix-cli/src/main.rshelix-cli/src/lib.rshelix-cli/Cargo.tomlhelix-cli/src/config.rshelix-cli/src/commands/mod.rshql-tests/tests/add_n/helix.tomlhql-tests/tests/user_test_1/schema.hxhql-tests/tests/user_test_1/helix.tomlhql-tests/tests/user_test_1/queries.hx.github/workflows/hql_tests.ymlSequence Diagram
sequenceDiagram participant User participant CLI as "Helix CLI" participant HelixManager participant SseClient participant StripeCheckout participant HelixCloud as "Helix Cloud API" User->>CLI: "helix create-cluster instance_name" CLI->>CLI: "Load project context & validate instance" CLI->>CLI: "Check authentication credentials" alt Not authenticated CLI-->>User: "Error: Not logged in" Note over User,CLI: "User must run 'helix auth login' first" end CLI->>HelixCloud: "POST /create-cluster (SSE stream)" HelixCloud-->>CLI: "SSE: CheckoutRequired {url}" CLI->>User: "Opening Stripe checkout..." CLI->>StripeCheckout: "Open browser to payment URL" User->>StripeCheckout: "Complete payment" StripeCheckout->>HelixCloud: "Payment confirmation webhook" HelixCloud-->>CLI: "SSE: PaymentConfirmed" CLI->>User: "Payment confirmed!" HelixCloud-->>CLI: "SSE: CreatingProject" CLI->>User: "Creating cluster..." HelixCloud-->>CLI: "SSE: ProjectCreated {cluster_id}" CLI->>CLI: "Update helix.toml with cluster_id" CLI->>User: "Cluster created successfully!"Context used:
dashboard- readme for helixdb (source)