feat: Output Polish: Rich Formatting with Colors, Boxes, Spinners #582
Conversation
fix: updated doctor.py ui, updated ui.py code
📝 WalkthroughWalkthroughThis PR introduces a new Rich-based UI module ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Summary of ChangesHello @lu11y0, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a significant polish of the application's command-line interface. By integrating the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great step forward in improving the user experience of the Cortex CLI by introducing rich, colorful, and dynamic output using the rich library. The new cortex/ui.py module is well-structured and provides a solid foundation for all terminal interactions. The refactoring of cortex/demo.py and cortex/doctor.py to use these new components makes the output much more professional and readable.
My feedback focuses on a few areas to further improve the code: removing artificial delays from the diagnostic tool, handling exceptions more safely, improving consistency in the use of new UI components, and some minor code cleanup. Overall, this is an excellent contribution.
|
will make this pr draft and update description after suggestions from bots 👍 |
![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
🤖 Fix all issues with AI agents
In @cortex/demo.py:
- Around line 89-92: The success message still includes the ellipsis because
step_msg.replace('... ', '') looks for '... ' (with a space) that doesn't exist;
update the loop that uses spinner and success (the for i, (step_msg, duration)
... block) to strip the trailing ellipsis from step_msg before passing to
success—e.g., detect if step_msg.endswith('...') and remove the last three
characters (or otherwise trim trailing dots) so success receives the cleaned
message.
In @cortex/doctor.py:
- Around line 392-396: Fix indentation on the self._print_check call in
cortex/doctor.py: align the third argument ("Upgrade RAM to at least 8GB") with
the other arguments by removing the extra leading space so the three arguments
inside the self._print_check(...) call are consistently indented; you can find
this call by locating the _print_check invocation that logs "Only {mem_gb:.1f}GB
RAM (8GB minimum required)".
In @cortex/ui.py:
- Around line 70-78: The docstring for the info function incorrectly states the
default for badge is True while the signature sets badge: bool = False; update
the docstring to reflect the actual default or change the function signature to
match the documented default. Locate the info function and either change the
docstring line "badge: Show CX badge (default: True)" to "badge: Show CX badge
(default: False)" or set the default parameter to True (badge: bool = True)
depending on intended behavior, ensuring consistency between the signature, the
docstring, and any callers that rely on BADGE/console printing.
🧹 Nitpick comments (7)
cortex/demo.py (2)
3-4: Redundant imports and duplicate Console instantiation.
ConsoleandPanelare imported directly fromrich, butcortex.uialready provides a sharedconsoleinstance. Creating a secondConsole()at line 17 leads to potential inconsistencies in output handling.♻️ Suggested fix
-from rich.console import Console -from rich.panel import Panel +from rich.panel import Panel from cortex.branding import show_banner from cortex.hardware_detection import detect_hardware -from cortex.ui import info, section, spinner, status_box, success, warning +from cortex.ui import console, info, section, spinner, status_box, success, warningThen remove line 17:
print() - console = Console() console.print("[bold cyan]🚀 Cortex One-Command Demo[/bold cyan]\n")Also applies to: 17-17
16-16: Inconsistent use ofprint()vsconsole.print().Mixing bare
print()withconsole.print()and Rich UI helpers can cause output buffering/ordering issues. For consistent terminal behavior with Rich's console management, preferconsole.print()or theinfo()helper throughout.Also applies to: 26-26, 40-40, 44-44, 57-57, 62-62, 80-80, 94-94, 96-96, 122-129
cortex/doctor.py (2)
6-27: Import ordering does not follow PEP 8.The
timeimport at line 27 should be grouped with standard library imports (lines 8-13). As per coding guidelines, PEP 8 style should be followed.♻️ Suggested fix
from __future__ import annotations import os import shutil import subprocess import sys +import time from pathlib import Path from typing import Optional from cortex.ui import ( console, section, spinner, success, warning, error, summary_box, info, ) from cortex.validators import validate_api_key from cortex.branding import show_banner -import time
440-461: Potentially redundant output in summary.
summary_boxat line 442 displays pass/warn/fail counts, then lines 452-461 re-display the same failures/warnings with additional formatting. This may be intentional for emphasis, but results in duplicate information being shown.Consider whether both outputs are needed, or if the detailed list could replace the count-only summary.
cortex/ui.py (3)
10-16: Unused import:Status.
Statusis imported fromrich.statusat line 13 but never used in the module. The spinner usesconsole.status()directly.♻️ Suggested fix
from rich.console import Console from rich.panel import Panel from rich.progress import Progress, SpinnerColumn, TextColumn, TimeElapsedColumn, BarColumn -from rich.status import Status from rich.table import Table from rich.columns import Columns
28-28: Missing return type annotations on public functions.Per coding guidelines, type hints are required. The following functions are missing
-> Nonereturn type annotations:success,error,warning,info,section,status_box,text_box,summary_box,compact_list,print_newline,clear,rule.Example fix for
success:-def success(message: str, details: str = "", badge: bool = True): +def success(message: str, details: str = "", badge: bool = True) -> None:Apply similarly to all other void functions.
Also applies to: 42-42, 56-56, 70-70, 81-81, 121-121, 174-174, 189-189, 292-292, 370-370, 376-376, 381-381
381-389: Local import inside function.
Ruleis imported inside the function while all other Rich imports are at module level. This is inconsistent but may be intentional ifrule()is rarely called.Consider moving the import to the top with other Rich imports for consistency:
+from rich.rule import Rule
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cortex/cli.pycortex/demo.pycortex/doctor.pycortex/ui.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/doctor.pycortex/demo.pycortex/ui.py
🧬 Code graph analysis (2)
cortex/demo.py (3)
cortex/branding.py (1)
show_banner(35-46)cortex/ui.py (6)
info(70-78)section(81-90)spinner(98-114)status_box(121-172)success(28-39)warning(56-67)cortex/hardware_detection.py (1)
total_gb(88-89)
cortex/ui.py (1)
cortex/cli.py (1)
status(1075-1089)
🔇 Additional comments (9)
cortex/demo.py (1)
11-14: LGTM!Function signature is properly typed with return type annotation, and the docstring describes the purpose. The demo flow structure with sections and spinners aligns well with the PR objectives for investor demos.
cortex/doctor.py (4)
75-99: Printing inside spinner context may cause visual artifacts.The spinner at line 75 wraps all diagnostic checks, but
_print_checkcalls Rich'sconsole.print()inside the spinner context. This can cause the spinner animation to be interrupted or overwritten by check output, potentially resulting in messy terminal display.Consider either:
- Using spinner only for silent operations, printing results after spinner stops
- Breaking into multiple smaller spinner/print cycles per section
This may work correctly if Rich handles nested output gracefully. Verify the visual behavior during actual execution.
112-117: Missing return type annotation.Per coding guidelines, type hints are required. The method should have
-> Nonereturn type annotation.♻️ Suggested fix
def _print_check( self, status: str, message: str, suggestion: Optional[str] = None, - ) -> None: + ) -> None:Wait, I see
-> Noneis already there at line 117. Let me re-check... Yes, line 117 shows) -> None:. This is correct.Actually, reviewing again - line 117 shows
-> None:is present. Disregard this comment.
30-56: LGTM!The
SystemDoctorclass has a comprehensive docstring documenting all checks performed and the tracked attributes. The constructor properly initializes the tracking lists.
270-282: LGTM!Good defensive coding with try/except guards around optional imports (
torch,psutil). The broad exception handling with silent pass is appropriate here since these are optional enhancement checks that shouldn't fail the entire diagnostic.Also applies to: 415-423
cortex/ui.py (4)
97-114: LGTM!The spinner context manager is correctly implemented with
@contextmanager, proper start/stop lifecycle, andtry/finallyensuring cleanup even on exceptions. Yielding the status object allows callers to update the message if needed.
225-289: LGTM!Well-documented function with proper return type annotation, comprehensive docstring with example, and flexible column configuration. The pattern of printing and returning the Table is useful for chaining or further manipulation.
1-22: LGTM!Clean module structure with clear section organization, well-named constants, and a shared console instance. This provides a cohesive API for the rich formatting requirements specified in issue #242.
337-363: LGTM!The progress bar context manager follows the same correct pattern as the spinner, with proper lifecycle management and a helpful usage example in the docstring.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cortex/ui.py:
- Around line 293-331: The parameter columns_count on compact_list is unused;
either remove it from the signature and docs (cleanup: delete columns_count and
any mentions in the docstring) or implement fixed-column behavior by chunking
items into rows of size columns_count and rendering each row with rich.Columns
(or building a rich.Table with columns_count columns) instead of passing all
panels at once; update the docstring and calls to compact_list accordingly and
keep the rest of the compact_list logic (creating Panel from each item and using
DEFAULT_BORDER_COLOR) intact.
🧹 Nitpick comments (5)
cortex/ui.py (3)
30-81: Consider adding explicit return type annotations.The status message functions (
success,error,warning,info) have parameter type hints but are missing explicit-> Nonereturn type annotations. This improves type checker coverage and self-documentation.✨ Suggested fix
-def success(message: str, details: str = "", badge: bool = True): +def success(message: str, details: str = "", badge: bool = True) -> None:Apply similarly to
error,warning, andinfo.
176-189: Minor: Docstring could include Args section for consistency.Other functions include detailed
Argssections, buttext_boxonly has a one-liner. Consider adding parameter documentation for consistency.
192-218: Parameter namesuccessshadows the module-level function.The parameter
success: boolshadows thesuccess()function defined at line 30. While not causing issues currently (sincesummary_boxdoesn't call thesuccessfunction internally), this can lead to confusion and potential bugs if the function is extended later.♻️ Suggested rename
def summary_box( - title: str, items: list[str], success: bool = True, border_color: str | None = None + title: str, items: list[str], is_success: bool = True, border_color: str | None = None ): ... - color = border_color or ("green" if success else "red") - icon = "✓" if success else "✗" + color = border_color or ("green" if is_success else "red") + icon = "✓" if is_success else "✗"cortex/doctor.py (2)
69-99: Artificial delays and spinner interaction may cause UX issues.
The
time.sleep(0.5)calls (lines 76, 87, 91, 95) add ~2 seconds of artificial delay to every health check. While this may be acceptable for investor demos, it could be frustrating in daily use or CI pipelines. Consider making this configurable (e.g., via an environment variable or parameter).The spinner wraps all checks, but checks output text inside the spinner context. Rich's spinner may not display cleanly when interleaved with other console output. Verify the visual output is as expected.
💡 Suggested approach for configurable delays
+DEMO_MODE = os.environ.get("CORTEX_DEMO_MODE", "").lower() == "true" + def run_checks(self) -> int: ... with spinner("[CX] Scanning system..."): - time.sleep(0.5) + if DEMO_MODE: + time.sleep(0.5)
313-332: Consider using the error message fromvalidate_api_key.The third return value from
validate_api_key()(the error message) is discarded with_on line 315. This message could provide more specific guidance to the user (e.g., "ANTHROPIC_API_KEY doesn't look valid" vs. generic "No API keys configured"). Consider using it in the warning:💡 Suggested improvement
- is_valid, provider, _ = validate_api_key() + is_valid, provider, error_msg = validate_api_key() ... # Otherwise warn self._print_check( "WARN", - "No API keys configured (required for cloud models)", - "Configure API key: export ANTHROPIC_API_KEY=sk-... or run 'cortex wizard'", + error_msg or "No API keys configured (required for cloud models)", + "Configure API key: export ANTHROPIC_API_KEY=sk-... or run 'cortex wizard'", )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/cli.pycortex/doctor.pycortex/ui.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/doctor.pycortex/ui.py
🧬 Code graph analysis (2)
cortex/doctor.py (3)
cortex/branding.py (1)
show_banner(35-46)cortex/ui.py (7)
error(44-55)info(72-80)section(83-92)spinner(101-117)success(30-41)summary_box(192-218)warning(58-69)cortex/validators.py (1)
validate_api_key(58-86)
cortex/ui.py (1)
cortex/cli.py (1)
status(1154-1168)
🔇 Additional comments (15)
cortex/ui.py (7)
1-23: LGTM!Module docstring, imports, and constants are well-organized. The Rich library imports cover all needed components for the UI helpers.
83-92: LGTM!Section header function is clean and well-documented.
100-117: LGTM!The spinner context manager is well-implemented with proper cleanup in the
finallyblock, ensuring the spinner stops even if an exception occurs.
125-173: LGTM!The
status_boxfunction provides good flexibility with thefitparameter for auto-sizing vs. fixed width panels. The key-value table rendering is clean.
226-290: LGTM!The
data_tablefunction is well-documented with a comprehensive docstring including return type. The implementation handles column configuration flexibly and safely converts cell values to strings.
339-365: LGTM!The
progress_barcontext manager provides a well-configured Rich progress bar with spinner, description, bar, percentage, and elapsed time columns. The cleanup infinallyensures proper resource management.
373-391: LGTM!Utility functions are simple and well-documented.
cortex/doctor.py (8)
6-27: LGTM!Imports are well-organized, grouping standard library, then project modules. The UI helpers imported match the new
cortex/ui.pymodule's public API.
30-55: LGTM!The
SystemDoctorclass has a comprehensive docstring explaining all health checks and attributes. The initialization is clean with properly typed list attributes.
108-110: LGTM!Clean delegation to the UI module's
sectionhelper.
112-141: LGTM!The
_print_checkmethod cleanly routes status messages to the appropriate UI helpers while maintaining internal tracking of passes, warnings, and failures. The suggestion handling is appropriate for each status type.
241-285: LGTM!Comments have been clarified and the
# type: ignorefor optionaltorchimport is appropriate.
287-311: LGTM!Minor comment improvements and appropriate
# type: ignorefor optionalrequestsimport.
395-420: LGTM!Return type annotation uses modern Python 3.10+ syntax (
float | None). The# type: ignorefor optionalpsutilimport is appropriate.
422-457: LGTM!The summary method provides a clear overview with counts via
summary_box, followed by actionable suggestions and detailed failure/warning listings. The structure is well-organized for user comprehension.
| def compact_list( | ||
| items: list[dict[str, str]], | ||
| title: str | None = None, | ||
| columns_count: int = 2, | ||
| equal: bool = True, | ||
| ): | ||
| """Display items in a compact multi-column layout | ||
| Args: | ||
| items: List of dicts with 'title', 'content', optional 'border_color' | ||
| title: Optional section title | ||
| columns_count: Number of columns (default: 2) | ||
| equal: Make columns equal width (default: True) | ||
| Example: | ||
| compact_list( | ||
| items=[ | ||
| {"title": "ML Stack", "content": "PyTorch, CUDA\\n6 packages", "border_color": "green"}, | ||
| {"title": "Web Stack", "content": "Node, npm\\n4 packages", "border_color": "blue"} | ||
| ], | ||
| title="Available Stacks", | ||
| columns_count=2 | ||
| ) | ||
| """ | ||
| if title: | ||
| section(title) | ||
|
|
||
| panels = [] | ||
| for item in items: | ||
| panel = Panel( | ||
| item["content"], | ||
| title=f"[bold]{item['title']}[/bold]", | ||
| border_style=item.get("border_color", DEFAULT_BORDER_COLOR), | ||
| expand=False, | ||
| ) | ||
| panels.append(panel) | ||
|
|
||
| console.print(Columns(panels, equal=equal, expand=True)) | ||
| console.print() |
There was a problem hiding this comment.
Unused parameter columns_count.
The columns_count parameter is declared but never used in the implementation. Rich's Columns automatically arranges items based on terminal width. Either remove the unused parameter or implement the intended functionality.
🔧 Option 1: Remove unused parameter
def compact_list(
items: list[dict[str, str]],
title: str | None = None,
- columns_count: int = 2,
equal: bool = True,
):🔧 Option 2: Implement column count control
If you want to control the number of columns, you may need to chunk the items manually or use a different layout approach, as Rich's Columns doesn't directly support a fixed column count.
📝 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.
| def compact_list( | |
| items: list[dict[str, str]], | |
| title: str | None = None, | |
| columns_count: int = 2, | |
| equal: bool = True, | |
| ): | |
| """Display items in a compact multi-column layout | |
| Args: | |
| items: List of dicts with 'title', 'content', optional 'border_color' | |
| title: Optional section title | |
| columns_count: Number of columns (default: 2) | |
| equal: Make columns equal width (default: True) | |
| Example: | |
| compact_list( | |
| items=[ | |
| {"title": "ML Stack", "content": "PyTorch, CUDA\\n6 packages", "border_color": "green"}, | |
| {"title": "Web Stack", "content": "Node, npm\\n4 packages", "border_color": "blue"} | |
| ], | |
| title="Available Stacks", | |
| columns_count=2 | |
| ) | |
| """ | |
| if title: | |
| section(title) | |
| panels = [] | |
| for item in items: | |
| panel = Panel( | |
| item["content"], | |
| title=f"[bold]{item['title']}[/bold]", | |
| border_style=item.get("border_color", DEFAULT_BORDER_COLOR), | |
| expand=False, | |
| ) | |
| panels.append(panel) | |
| console.print(Columns(panels, equal=equal, expand=True)) | |
| console.print() | |
| def compact_list( | |
| items: list[dict[str, str]], | |
| title: str | None = None, | |
| equal: bool = True, | |
| ): | |
| """Display items in a compact multi-column layout | |
| Args: | |
| items: List of dicts with 'title', 'content', optional 'border_color' | |
| title: Optional section title | |
| equal: Make columns equal width (default: True) | |
| Example: | |
| compact_list( | |
| items=[ | |
| {"title": "ML Stack", "content": "PyTorch, CUDA\\n6 packages", "border_color": "green"}, | |
| {"title": "Web Stack", "content": "Node, npm\\n4 packages", "border_color": "blue"} | |
| ], | |
| title="Available Stacks" | |
| ) | |
| """ | |
| if title: | |
| section(title) | |
| panels = [] | |
| for item in items: | |
| panel = Panel( | |
| item["content"], | |
| title=f"[bold]{item['title']}[/bold]", | |
| border_style=item.get("border_color", DEFAULT_BORDER_COLOR), | |
| expand=False, | |
| ) | |
| panels.append(panel) | |
| console.print(Columns(panels, equal=equal, expand=True)) | |
| console.print() |
🤖 Prompt for AI Agents
In @cortex/ui.py around lines 293 - 331, The parameter columns_count on
compact_list is unused; either remove it from the signature and docs (cleanup:
delete columns_count and any mentions in the docstring) or implement
fixed-column behavior by chunking items into rows of size columns_count and
rendering each row with rich.Columns (or building a rich.Table with
columns_count columns) instead of passing all panels at once; update the
docstring and calls to compact_list accordingly and keep the rest of the
compact_list logic (creating Panel from each item and using
DEFAULT_BORDER_COLOR) intact.
|
Superseded by #595 Rich output formatting. Thank you! |
2 participants
Related Issue
Closes #242
Summary
AI Disclosure
Checklist
type(scope): descriptionor[scope] descriptionpytest tests/)Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.