-
-
Notifications
You must be signed in to change notification settings - Fork 169
fixed spinner issue-705 #706
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.
3 files reviewed, 6 comments
helix-cli/src/commands/build.rs
Outdated
| spinner::with_spinner( | ||
| "Caching Helix repository...", | ||
| ensure_helix_repo_cached(), | ||
| ) | ||
| .await?; |
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: spinner output will conflict with print_status calls inside ensure_helix_repo_cached() (lines 125, 145, 149, 162, 175). The spinner uses \r to overwrite lines, but internal prints will create new lines causing jumbled output
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-cli/src/commands/build.rs
Line: 49:53
Comment:
**logic:** spinner output will conflict with `print_status` calls inside `ensure_helix_repo_cached()` (lines 125, 145, 149, 162, 175). The spinner uses `\r` to overwrite lines, but internal prints will create new lines causing jumbled output
How can I resolve this? If you propose a fix, please make it concise.
helix-cli/src/commands/build.rs
Outdated
| spinner::with_spinner( | ||
| &format!("Preparing workspace for '{instance_name}'..."), | ||
| prepare_instance_workspace(&project, &instance_name), | ||
| ) | ||
| .await?; |
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: spinner will be disrupted by print_status calls inside prepare_instance_workspace() at lines 242 and 263
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-cli/src/commands/build.rs
Line: 56:60
Comment:
**logic:** spinner will be disrupted by `print_status` calls inside `prepare_instance_workspace()` at lines 242 and 263
How can I resolve this? If you propose a fix, please make it concise.
helix-cli/src/commands/build.rs
Outdated
| let compile_result = spinner::with_spinner( | ||
| "Compiling Helix queries...", | ||
| compile_project(&project, &instance_name), | ||
| ) | ||
| .await; |
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: spinner will be disrupted by multiple print_status calls inside compile_project() at lines 272, 289, 338, 344
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-cli/src/commands/build.rs
Line: 63:67
Comment:
**logic:** spinner will be disrupted by multiple `print_status` calls inside `compile_project()` at lines 272, 289, 338, 344
How can I resolve this? If you propose a fix, please make it concise.
helix-cli/src/commands/build.rs
Outdated
| spinner::with_spinner( | ||
| &format!("Building Docker image for '{instance_name}'..."), | ||
| async { | ||
| docker.build_image(&instance_name, instance_config.docker_build_target())?; | ||
| Ok::<(), eyre::Error>(()) | ||
| }, | ||
| ) | ||
| .await?; |
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: spinner will be disrupted by print_status calls inside docker.build_image() at lines 262 and 274 in docker.rs
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-cli/src/commands/build.rs
Line: 107:114
Comment:
**logic:** spinner will be disrupted by `print_status` calls inside `docker.build_image()` at lines 262 and 274 in `docker.rs`
How can I resolve this? If you propose a fix, please make it concise.
helix-cli/src/commands/push.rs
Outdated
| spinner::with_spinner( | ||
| &format!("Starting instance '{instance_name}'..."), | ||
| async { | ||
| docker.start_instance(instance_name)?; | ||
| Ok::<(), eyre::Error>(()) | ||
| }, | ||
| ) | ||
| .await?; |
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: spinner will be disrupted by print_status calls inside docker.start_instance() at lines 280 and 289 in docker.rs
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-cli/src/commands/push.rs
Line: 108:115
Comment:
**logic:** spinner will be disrupted by `print_status` calls inside `docker.start_instance()` at lines 280 and 289 in `docker.rs`
How can I resolve this? If you propose a fix, please make it concise.
helix-cli/src/utils.rs
Outdated
| // Clear the line when done | ||
| print!("\r{}", " ".repeat(message.len() + 3)); | ||
| print!("\r"); | ||
| let _ = stdout.flush(); |
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: clearing the line with spaces doesn't account for terminal width - if message is shorter than previous output, remnants may remain. Consider using ANSI escape codes like \x1b[2K to clear the entire line
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-cli/src/utils.rs
Line: 260:263
Comment:
**style:** clearing the line with spaces doesn't account for terminal width - if message is shorter than previous output, remnants may remain. Consider using ANSI escape codes like `\x1b[2K` to clear the entire line
How can I resolve this? If you propose a fix, please make it concise.|
@megaketu555 thoughts on greptile comments? |
|
@xav-db Hi, thanks for taking your time to review the PR. I have fixed most of the issues as mentioned by the greptile bot. if there is anyone remaining do lmk |
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.
3 files reviewed, 3 comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
| spinner::with_spinner( | ||
| "Deploying to Fly.io...", | ||
| async { | ||
| let fly = FlyManager::new(project, config.auth_type.clone()).await?; | ||
| let docker = DockerManager::new(project); | ||
| // Get the correct image name from docker compose project name | ||
| let image_name = docker.image_name(instance_name, config.build_mode); | ||
|
|
||
| fly.deploy_image(&docker, config, instance_name, &image_name) | ||
| .await?; | ||
| Ok::<(), eyre::Error>(()) | ||
| }, | ||
| ) | ||
| .await?; |
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: spinner will be disrupted by print_status calls inside fly.deploy_image() at lines 397, 406, 418, and 445 in fly.rs. The spinner uses \r to overwrite lines, but internal prints create new lines causing jumbled output
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-cli/src/commands/push.rs
Line: 156:169
Comment:
**logic:** spinner will be disrupted by `print_status` calls inside `fly.deploy_image()` at lines 397, 406, 418, and 445 in `fly.rs`. The spinner uses `\r` to overwrite lines, but internal prints create new lines causing jumbled output
How can I resolve this? If you propose a fix, please make it concise.| spinner::with_spinner( | ||
| "Deploying to ECR...", | ||
| async { | ||
| let ecr = EcrManager::new(project, config.auth_type.clone()).await?; | ||
| let docker = DockerManager::new(project); | ||
| // Get the correct image name from docker compose project name | ||
| let image_name = docker.image_name(instance_name, config.build_mode); | ||
|
|
||
| ecr.deploy_image(&docker, config, instance_name, &image_name) | ||
| .await?; | ||
| Ok::<(), eyre::Error>(()) | ||
| }, | ||
| ) | ||
| .await?; |
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: spinner will be disrupted by print_status calls inside ecr.deploy_image() at lines 325, 331, 365, 370, and 383 in ecr.rs
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-cli/src/commands/push.rs
Line: 172:185
Comment:
**logic:** spinner will be disrupted by `print_status` calls inside `ecr.deploy_image()` at lines 325, 331, 365, 370, and 383 in `ecr.rs`
How can I resolve this? If you propose a fix, please make it concise.| spinner::with_spinner( | ||
| "Deploying to Helix Cloud...", | ||
| async { | ||
| let helix = HelixManager::new(project); | ||
| helix.deploy(None, instance_name.to_string()).await?; | ||
| Ok::<(), eyre::Error>(()) | ||
| }, | ||
| ) | ||
| .await?; |
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: spinner will be disrupted by print_status calls inside helix.deploy() at lines 65, 86, 90, 94, and 138 in helix.rs
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-cli/src/commands/push.rs
Line: 188:196
Comment:
**logic:** spinner will be disrupted by `print_status` calls inside `helix.deploy()` at lines 65, 86, 90, 94, and 138 in `helix.rs`
How can I resolve this? If you propose a fix, please make it concise.|
@megaketu555 there seems to be a number of issues with prints being in spinners. if you could also show a screenshot of the spinner for my engineers to see this would be helpful! |
|
Sorry for the late response. I have updated the push.rs file such that only pushing to the cloud seems to trigger spinner. so I wrote a little script to trigger spinners Screen.Recording.2025-11-23.031403.mp4 |

Description
Implemented the spinner when building instances and deploying instances
Related Issues
#705
Closes #
Checklist when merging to main
rustfmthelix-cli/Cargo.tomlandhelixdb/Cargo.tomlAdditional Notes
Greptile Summary
parse_queries_for_metrics()to use shared utility functions fromhelixc_utilsImportant Files Changed
print_statuscalls that will conflict with spinner's line-overwriting mechanismSequence Diagram
sequenceDiagram participant User participant CLI participant Spinner participant DeployFunc participant stdout User->>CLI: "helix push cloud-instance" CLI->>Spinner: "with_spinner(message, deploy_image)" activate Spinner Spinner->>Spinner: "spawn animation task" par Spinner Animation loop "Every 100ms" Spinner->>stdout: "\r + spinner_char + message" end and Operation Execution Spinner->>DeployFunc: "await fly.deploy_image()" activate DeployFunc DeployFunc->>stdout: "print_status() calls" Note over Spinner,stdout: "Output conflicts with spinner" DeployFunc-->>Spinner: "Result" deactivate DeployFunc end Spinner->>Spinner: "stop animation task" Spinner->>stdout: "\r\x1b[2K (clear line)" deactivate Spinner Spinner-->>CLI: "operation result" CLI-->>User: "success/error message"