Feat: TUI dashboard fix Interactive TUI Dashboard #277
Conversation
|
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. WalkthroughAdds a new terminal dashboard feature and CLI entry point: implements a Rich-based TUI (live system metrics, process list, command history, install/bench/doctor actions), integrates it into Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CortexCLI
participant App as DashboardApp
participant Monitor as SystemMonitor
participant Lister as ProcessLister
participant History as CommandHistory
participant UI as UIRenderer
User->>CLI: cortex dashboard
CLI->>App: DashboardApp()
App->>Monitor: init & start monitoring
App->>Lister: init & start process listing
App->>History: load command history
App->>UI: UIRenderer(monitor,lister,history).run()
loop Dashboard loop
UI->>Monitor: get_metrics()
Monitor-->>UI: SystemMetrics
UI->>Lister: get_processes()
Lister-->>UI: process list
UI->>User: render (home/progress)
User->>UI: keyboard input (q/Tab/1-4/Enter/Esc)
alt action triggers background task
UI->>App: spawn bench/install/doctor task
App->>UI: progress updates
end
end
User->>UI: q
UI->>App: cleanup & stop
UI-->>User: exit
sequenceDiagram
participant User
participant UI as UIRenderer
participant Progress as InstallationProgress
participant Worker as BackgroundWorker
User->>UI: press 2 (start bench)
UI->>Progress: set IN_PROGRESS / BENCH
UI->>Worker: spawn async bench
loop bench steps
Worker->>Progress: update step / elapsed
Worker->>UI: push progress
UI->>User: render progress
end
Worker->>Progress: set COMPLETED & results
UI->>User: show results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: 4
🧹 Nitpick comments (17)
requirements-dev.txt (1)
23-26: Remove duplicate dependencies already included viarequirements.txt.Since
requirements.txtis included via-r requirements.txton line 4, andpsutil>=5.0.0andpynvml>=11.0.0are already added there, these lines are redundant. Duplicating version constraints risks divergence if updated in only one place.Apply this diff:
- -# System monitoring (for dashboard) -psutil>=5.0.0 -pynvml>=11.0.0cortex/cli.py (2)
751-768: Dashboard method implementation looks good with minor refinements possible.The error handling appropriately catches
ImportErrorfor missing dependencies and provides helpful guidance. TheKeyboardInterrupthandling correctly returns 0 for user-initiated exit.Consider logging the exception details in verbose mode for debugging purposes:
def dashboard(self): """Launch the real-time system monitoring dashboard""" try: from cortex.dashboard import DashboardApp app = DashboardApp() app.run() return 0 except ImportError as e: self._print_error(f"Dashboard dependencies not available: {e}") cx_print("Install required packages with:", "info") cx_print(" pip install psutil pynvml", "info") return 1 except KeyboardInterrupt: return 0 except Exception as e: + self._debug(f"Dashboard exception: {type(e).__name__}: {e}") self._print_error(f"Dashboard error: {e}") return 1
843-845: Suppress unused variable warning by using_prefix.The static analysis correctly identifies
dashboard_parseras unused. Since no arguments are added to this subparser, use_prefix to indicate this is intentional.# Dashboard command - dashboard_parser = subparsers.add_parser('dashboard', help='Real-time system monitoring dashboard') + _dashboard_parser = subparsers.add_parser('dashboard', help='Real-time system monitoring dashboard')Alternatively, simply don't assign the result if the reference isn't needed:
subparsers.add_parser('dashboard', help='Real-time system monitoring dashboard')test/test_dashboard.py (3)
9-15: Consider using pytest fixtures instead of repeated dynamic imports.The
load_dashboard()function is called at the start of every test, re-executing the module each time. This is inefficient and bypasses pytest's fixture system.+import pytest + +@pytest.fixture(scope="module") +def dashboard(): + """Load dashboard module once for all tests""" path = os.path.join(os.path.dirname(os.path.dirname(__file__)), "cortex", "dashboard.py") spec = importlib.util.spec_from_file_location("dashboard", path) - dashboard = importlib.util.module_from_spec(spec) + module = importlib.util.module_from_spec(spec) - spec.loader.exec_module(dashboard) + spec.loader.exec_module(module) - return dashboard + return moduleThen update tests to accept the fixture:
def test_system_monitor(dashboard): monitor = dashboard.SystemMonitor() # ...
76-86: Prefer testing public interface over private methods.Testing
_render_*methods directly couples tests to implementation details. If the internal structure changes, tests will break even if public behavior remains correct. Consider testing the public_render_screen()output or the final rendered result.Also, the
all()assertion obscures which component failed:- assert all([header, resources, processes, hist, actions, footer, screen]), \ - "All components should render" + assert header, "Header should render" + assert resources, "Resources should render" + assert processes, "Processes should render" + assert hist, "History should render" + assert actions, "Actions should render" + assert footer, "Footer should render" + assert screen, "Screen should render"
113-148: Consider converting to standard pytest format.Since
pytest>=7.0.0is already in dev dependencies, the custom test runner is redundant. Pytest provides better reporting, parallelization, fixtures, and CI integration out of the box.The tests can be run directly with
pytest test/test_dashboard.py -vwithout the custommain()runner.If you want to keep the standalone runner capability, you can use:
if __name__ == "__main__": import pytest sys.exit(pytest.main([__file__, "-v"]))docs/DASHBOARD_IMPLEMENTATION.md (2)
51-73: Add language specifier to fenced code blocks for better rendering.The ASCII diagram and other code blocks lack language specifiers, which affects syntax highlighting in some renderers. Use
textorplaintextfor diagrams:-``` +```text ┌─────────────────────────────────────────────────────┐ │ DashboardApp (Main Orchestrator) │This also applies to other code blocks at lines 93, 121, 129, 275, 296, 316, 338, 357, 370, and 706.
731-733: Convert bare URLs to markdown links.Use proper markdown link syntax for consistency and better accessibility:
-- **Rich Library:** https://rich.readthedocs.io/ -- **psutil:** https://psutil.readthedocs.io/ -- **NVIDIA NVML (pynvml):** https://docs.nvidia.com/cuda/nvml-api/ +- **Rich Library:** [rich.readthedocs.io](https://rich.readthedocs.io/) +- **psutil:** [psutil.readthedocs.io](https://psutil.readthedocs.io/) +- **NVIDIA NVML (pynvml):** [NVML API Docs](https://docs.nvidia.com/cuda/nvml-api/)cortex/dashboard.py (9)
28-34: Chain exceptions for better debuggability.Re-raising
ImportErrorwithout chaining loses the original traceback context. Useraise ... from eto preserve the exception chain.except ImportError as e: - raise ImportError(f"rich library required: {e}. Install with: pip install rich") + raise ImportError(f"rich library required: {e}. Install with: pip install rich") from e try: import psutil except ImportError as e: - raise ImportError(f"psutil library required: {e}. Install with: pip install psutil") + raise ImportError(f"psutil library required: {e}. Install with: pip install psutil") from e
177-178: Uselogging.exceptionto capture stack traces.When logging errors in exception handlers,
logging.exceptionautomatically includes the stack trace, which aids debugging.except Exception as e: - logger.error(f"Metrics error: {e}") + logger.exception("Metrics error: %s", e)
184-188: Usefrozensetfor immutable class constant.
KEYWORDSis a class-level constant that shouldn't be mutated. Usingfrozensetmakes immutability explicit and satisfies the static analysis warning about mutable class attributes.- KEYWORDS = { + KEYWORDS: frozenset[str] = frozenset({ 'python', 'node', 'ollama', 'llama', 'bert', 'gpt', 'transformers', 'inference', 'pytorch', 'tensorflow', 'cortex', 'cuda' - } + })Also note that
'python'and'node'are very broad keywords that may match many unrelated processes. Consider whether more specific filtering is needed.
215-216: Uselogging.exceptionfor error logging in exception handlers.except Exception as e: - logger.error(f"Process listing error: {e}") + logger.exception("Process listing error: %s", e)
371-372: Rename unused loop variable.The
action_typevariable is not used within the loop body. Rename it to_action_typeto indicate it's intentionally unused.- for key, name, action_type in action_items: + for key, name, _action_type in action_items: actions.append(f"[cyan]{key}[/cyan] {name}")
402-467: Consider refactoring to reduce cognitive complexity.SonarCloud flags this method with cognitive complexity of 19 (allowed: 15). Consider extracting helper methods for distinct rendering concerns:
You could extract:
- Progress bar rendering (lines 416-421) into
_render_progress_bar()- Doctor results rendering (lines 432-436) into
_render_doctor_results()- Status messages rendering (lines 445-448) into
_render_status_messages()This would improve readability and testability while reducing complexity.
734-788: Refactorrun()method to reduce cognitive complexity.SonarCloud flags cognitive complexity of 19 (allowed: 15). The method handles terminal setup, threading, keyboard input, and cleanup in one place.
Consider extracting:
- Terminal setup/teardown into context manager or helper methods
- The main loop body into a separate method
Also, the try-except-pass blocks at lines 745-746 and 786-787 silently swallow errors. At minimum, log these at debug level:
try: old_settings = termios.tcgetattr(sys.stdin) tty.setcbreak(sys.stdin.fileno()) - except Exception: - pass + except Exception as e: + logger.debug("Failed to set terminal mode: %s", e)
758-759: Uselogging.exceptionfor error logging.except Exception as e: - logger.error(f"Monitor error: {e}") + logger.exception("Monitor error: %s", e)
50-52: Avoid global logging configuration in module.
logging.basicConfig()configures the root logger and affects all modules. This could interfere with logging configuration set by the main application or other libraries.Configure only this module's logger instead:
-# Suppress verbose logging -logging.basicConfig(level=logging.WARNING) logger = logging.getLogger(__name__) +logger.setLevel(logging.WARNING)Or better, let the application's entry point configure logging globally.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cortex/cli.py(3 hunks)cortex/dashboard.py(1 hunks)docs/DASHBOARD_IMPLEMENTATION.md(1 hunks)requirements-dev.txt(1 hunks)requirements.txt(1 hunks)test/test_dashboard.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cortex/cli.py (2)
cortex/dashboard.py (2)
run(734-787)run(799-813)cortex/branding.py (1)
cx_print(55-75)
cortex/dashboard.py (1)
src/progress_tracker.py (1)
elapsed_time(69-74)
🪛 GitHub Check: SonarCloud Code Analysis
cortex/cli.py
[warning] 844-844: Remove the unused local variable "dashboard_parser".
cortex/dashboard.py
[failure] 402-402: Refactor this function to reduce its Cognitive Complexity from 19 to the 15 allowed.
[failure] 734-734: Refactor this function to reduce its Cognitive Complexity from 19 to the 15 allowed.
[warning] 89-89: Replace the type hint "datetime" with "Optional[datetime]" or don't assign "None" to "timestamp"
🪛 LanguageTool
docs/DASHBOARD_IMPLEMENTATION.md
[style] ~757-~757: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...g | --- Last Updated: December 8, 2025 Status: ✅ Complete and Tested *...
(MISSING_COMMA_AFTER_YEAR)
🪛 markdownlint-cli2 (0.18.1)
docs/DASHBOARD_IMPLEMENTATION.md
51-51: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
121-121: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
129-129: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
275-275: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
296-296: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
316-316: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
338-338: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
357-357: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
370-370: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
706-706: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
731-731: Bare URL used
(MD034, no-bare-urls)
732-732: Bare URL used
(MD034, no-bare-urls)
733-733: Bare URL used
(MD034, no-bare-urls)
🪛 Ruff (0.14.7)
test/test_dashboard.py
135-135: Do not catch blind exception: Exception
(BLE001)
cortex/cli.py
758-758: Consider moving this statement to an else block
(TRY300)
766-766: Do not catch blind exception: Exception
(BLE001)
844-844: Local variable dashboard_parser is assigned to but never used
Remove assignment to unused variable dashboard_parser
(F841)
cortex/dashboard.py
29-29: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
29-29: Avoid specifying long messages outside the exception class
(TRY003)
34-34: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
34-34: Avoid specifying long messages outside the exception class
(TRY003)
138-138: Do not catch blind exception: Exception
(BLE001)
163-163: Do not catch blind exception: Exception
(BLE001)
177-177: Do not catch blind exception: Exception
(BLE001)
178-178: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
184-188: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
215-215: Do not catch blind exception: Exception
(BLE001)
216-216: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
247-247: Do not catch blind exception: Exception
(BLE001)
371-371: Loop control variable action_type not used within loop body
Rename unused action_type to _action_type
(B007)
730-730: Do not catch blind exception: Exception
(BLE001)
745-746: try-except-pass detected, consider logging the exception
(S110)
745-745: Do not catch blind exception: Exception
(BLE001)
758-758: Do not catch blind exception: Exception
(BLE001)
759-759: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
786-787: try-except-pass detected, consider logging the exception
(S110)
786-786: Do not catch blind exception: Exception
(BLE001)
809-809: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (7)
cortex/cli.py (1)
894-895: LGTM!The command routing follows the established pattern and correctly returns the exit code from
cli.dashboard().test/test_dashboard.py (1)
48-59: Test may cause state pollution by adding commands to history.The test calls
history.add_command("test")which may persist beyond the test run ifCommandHistorywrites to disk. This could pollute subsequent test runs or actual user history.Consider mocking the history or using a temporary/isolated history file for tests.
requirements.txt (1)
13-15: Consider makingpynvmlan optional dependency.
pynvmlrequires NVIDIA GPU drivers and will fail to install or import on systems without NVIDIA hardware (AMD GPUs, Intel-only, ARM devices). Since the dashboard gracefully degrades when GPU monitoring is unavailable, consider:
- Moving
pynvmlto an optional extras group (e.g., viaextras_requirein setup.py/pyproject.toml), or- Documenting that installation may fail on non-NVIDIA systems and users can safely ignore or skip it.
cortex/dashboard.py (4)
55-78: LGTM!The enum definitions are clean, well-documented, and appropriately cover the dashboard states and actions.
224-260: LGTM!The CommandHistory class handles shell history loading gracefully with proper error handling and thread-safe access. The deque with maxlen is a good choice for bounded storage.
790-814: LGTM!The DashboardApp class provides a clean entry point with proper error handling and cleanup. The 1-second startup delay (line 805) provides visual feedback to the user.
816-823: LGTM!Clean entry point with proper
__main__guard.
There was a problem hiding this comment.
Pull request overview
This PR adds a terminal-based interactive dashboard feature to Cortex that provides real-time system monitoring (CPU, RAM, GPU), process tracking, and command history display. The dashboard integrates keyboard navigation for switching between tabs and executing actions like package installation, system benchmarks, and health checks. The implementation uses the Rich library for UI rendering, psutil for system metrics, and pynvml for optional GPU monitoring.
Key Changes
- Adds new
cortex dashboardCLI command that launches an interactive TUI - Implements real-time system resource monitoring with configurable refresh rates
- Provides multi-tab interface with keyboard navigation (Tab, 1-4, q keys) for installation, benchmarking, and system diagnostics
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 45 comments.
Show a summary per file
| File | Description |
|---|---|
| cortex/dashboard.py | Core dashboard implementation with SystemMonitor, ProcessLister, CommandHistory, UIRenderer, and DashboardApp classes (823 lines) |
| cortex/cli.py | Adds dashboard() method and argument parser integration for new command |
| requirements.txt | Adds psutil>=5.0.0 and pynvml>=11.0.0 dependencies for system monitoring |
| requirements-dev.txt | Duplicates system monitoring dependencies (should reference requirements.txt instead) |
| test/test_dashboard.py | Test suite using standalone functions instead of unittest.TestCase pattern (inconsistent with project conventions) |
| docs/DASHBOARD_IMPLEMENTATION.md | Comprehensive 760-line implementation guide with architecture details, usage examples, and troubleshooting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
test/test_dashboard.py (2)
20-147: Align test structure with the rest of the suite (unittest/TestCase vs custom main)This module uses free functions and a custom
main()loop rather than theunittest.TestCasepattern used elsewhere intest/, which was already raised in earlier review comments. Consider refactoring into aTestDashboardtest case so it plugs cleanly into the existing runner and remains consistent.
64-97: Strengthen UI renderer assertions and add interaction coverage
test_ui_renderer()only assertsall([header, resources, ...]), which will always be truthy for non-None objects and doesn’t validate content or behavior. It also doesn’t exercise keyboard-driven flows (Tab, 1–4, input mode) or progress/doctor/bench workflows that are core to this feature and previously flagged as missing.- # Test rendering - header = ui._render_header() - resources = ui._render_resources() - processes = ui._render_processes() - hist = ui._render_history() - actions = ui._render_actions() - footer = ui._render_footer() - screen = ui._render_screen() - - assert all([header, resources, processes, hist, actions, footer, screen]), \ - "All components should render" + # Test rendering + header = ui._render_header() + resources = ui._render_resources() + processes = ui._render_processes() + hist = ui._render_history() + actions = ui._render_actions() + footer = ui._render_footer() + screen = ui._render_screen() + + for name, component in { + "header": header, + "resources": resources, + "processes": processes, + "history": hist, + "actions": actions, + "footer": footer, + "screen": screen, + }.items(): + assert len(str(component)) > 0, f"{name} should have content" + + # Follow‑up: consider tests that simulate key input and verify: + # - Tab cycles HOME/PROGRESS tabs + # - Keys 1–4 start/cancel install/bench/doctor + # - Input mode correctly handles Enter/Escape/Backspace and character input + # - Progress state is updated as expected for install/bench/doctor flowsIf you’d like, I can sketch concrete tests that drive
_handle_key_pressand assert oncurrent_tab,installation_progress.state, and thedoctor_results/bench_statusfields.cortex/dashboard.py (1)
150-159: Ensure NVML is properly shut down to avoid GPU resource leaks
SystemMonitor._init_gpu()callspynvml.nvmlInit()but there’s no corresponding shutdown, so the NVML handle is never released. Adding a smallshutdown()hook and invoking it fromDashboardAppcleanup will address this (this was also mentioned in earlier reviews).class SystemMonitor: @@ def _init_gpu(self): """Initialize GPU monitoring if available""" if not GPU_AVAILABLE or pynvml is None: return try: pynvml.nvmlInit() self.gpu_initialized = True except Exception as e: logger.debug(f"GPU init failed: {e}") + + def shutdown(self) -> None: + """Release GPU monitoring resources if initialized.""" + if self.gpu_initialized and pynvml is not None: + try: + pynvml.nvmlShutdown() + except Exception as e: + logger.debug(f"GPU shutdown error: {e}") + finally: + self.gpu_initialized = False @@ def run(self): """Run the app""" console = Console() try: @@ except Exception as e: console.print(f"[red]Error: {e}[/red]") finally: self.ui.running = False + # Ensure system monitor resources (e.g. NVML) are cleaned up. + try: + self.monitor.shutdown() + except Exception: + logger.debug("Failed to shutdown SystemMonitor cleanly") console.print("\n[yellow]Dashboard shutdown[/yellow]")Please double‑check NVML shutdown behavior against the
pynvmldocs in your target environment.Also applies to: 830-844
🧹 Nitpick comments (5)
test/test_dashboard.py (1)
1-17: Simplify dashboard loading and avoid manual spec/path logic
load_dashboard()plus thesys.path.insert()andspec_from_file_locationdance are more complex and brittle than needed; you can load the module via its package path and let Python handle resolution.-import sys -import os -import importlib.util +import sys +import os +import importlib @@ -# Add parent directory to path -sys.path.insert(0, os.path.dirname(os.path.dirname(__file__))) +# Ensure project root (containing the `cortex` package) is on sys.path +sys.path.insert(0, os.path.dirname(os.path.dirname(__file__))) @@ -def load_dashboard(): - """Load dashboard module""" - path = os.path.join(os.path.dirname(os.path.dirname(__file__)), "cortex", "dashboard.py") - spec = importlib.util.spec_from_file_location("dashboard", path) - if spec is None or spec.loader is None: - raise ImportError("Failed to load dashboard module") - dashboard = importlib.util.module_from_spec(spec) - spec.loader.exec_module(dashboard) - return dashboard +def load_dashboard(): + """Load dashboard module via the installed/importable package.""" + return importlib.import_module("cortex.dashboard")cortex/dashboard.py (4)
200-208: AnnotateKEYWORDSas aClassVarto satisfy mutability/type-checking rules
ProcessLister.KEYWORDSis a mutable class attribute and Ruff flagged it; marking it asClassVar[Set[str]]clarifies intent and silences the warning.-from typing import Dict, List, Optional, Tuple, Callable +from typing import ClassVar, Dict, List, Optional, Tuple, Callable, Set @@ class ProcessLister: """Lists running inference processes""" - KEYWORDS = { + KEYWORDS: ClassVar[Set[str]] = { 'python', 'node', 'ollama', 'llama', 'bert', 'gpt', 'transformers', 'inference', 'pytorch', 'tensorflow', - 'cortex', 'cuda' - } + 'cortex', 'cuda', + }Also applies to: 11-11
384-395: Clean up unused loop variable in_render_actionsThe
action_typeloop variable is unused, which Ruff flagged; renaming it to_action_type(or dropping it) makes the intent explicit without behavior change.- for key, name, action_type in action_items: + for key, name, _action_type in action_items: actions.append(f"[cyan]{key}[/cyan] {name}")
165-197: Uselogger.exceptionwhere you’re already catching broad exceptionsIn
SystemMonitor.update_metrics,ProcessLister.update_processes, andmonitor_loop(), you catchExceptionand log withlogger.error(f"... {e}"). Usinglogger.exception()will capture stack traces and makes debugging runtime issues easier without changing behavior.def update_metrics(self): @@ - except Exception as e: - logger.error(f"Metrics error: {e}") + except Exception: + logger.exception("Metrics error") @@ def update_processes(self): @@ - except Exception as e: - logger.error(f"Process listing error: {e}") + except Exception: + logger.exception("Process listing error") @@ def monitor_loop(): while self.running: try: self.monitor.update_metrics() self.lister.update_processes() @@ - except Exception as e: - logger.error(f"Monitor error: {e}") + except Exception: + logger.exception("Monitor error") time.sleep(1.0)Also applies to: 213-235, 769-781
423-488: Consider splitting_render_progress_panelandrun()to reduce complexitySonar is flagging both
_render_progress_panelandUIRenderer.run()for cognitive complexity; both methods mix several responsibilities (formatting text, mapping states to titles, time/status handling, keyboard loop, and thread/terminal management).Possible decompositions (no behavior change):
- For
_render_progress_panel:
- Extract helpers like
_format_progress_bar(),_format_doctor_results(),_format_libraries_section(), and_format_status_messages()and assemble their returns.- For
run():
- Move
monitor_loopto a private method, and/or extract_render_loop(live)that contains the keyboard read +live.update()logic.This will make it easier to evolve the dashboard (e.g., add more operations/tabs) without fighting complex monolithic methods.
Also applies to: 754-809
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/dashboard.py(1 hunks)requirements.txt(1 hunks)test/test_dashboard.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
🧰 Additional context used
🧬 Code graph analysis (1)
test/test_dashboard.py (1)
cortex/dashboard.py (19)
SystemMonitor(120-197)update_metrics(165-197)get_metrics(160-163)ProcessLister(200-240)update_processes(213-235)get_processes(237-240)CommandHistory(243-278)get_history(275-278)add_command(269-273)UIRenderer(281-808)_render_header(327-341)_render_resources(343-357)_render_processes(359-368)_render_history(370-379)_render_actions(381-401)_render_footer(501-504)_render_screen(506-518)DashboardApp(811-844)main(847-850)
🪛 GitHub Check: SonarCloud Code Analysis
cortex/dashboard.py
[failure] 754-754: Refactor this function to reduce its Cognitive Complexity from 20 to the 15 allowed.
[failure] 423-423: Refactor this function to reduce its Cognitive Complexity from 19 to the 15 allowed.
🪛 Ruff (0.14.7)
test/test_dashboard.py
14-14: Avoid specifying long messages outside the exception class
(TRY003)
137-137: Do not catch blind exception: Exception
(BLE001)
cortex/dashboard.py
27-27: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
27-27: Avoid specifying long messages outside the exception class
(TRY003)
32-32: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
32-32: Avoid specifying long messages outside the exception class
(TRY003)
157-157: Do not catch blind exception: Exception
(BLE001)
182-182: Do not catch blind exception: Exception
(BLE001)
196-196: Do not catch blind exception: Exception
(BLE001)
197-197: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
203-207: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
234-234: Do not catch blind exception: Exception
(BLE001)
235-235: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
266-266: Do not catch blind exception: Exception
(BLE001)
392-392: Loop control variable action_type not used within loop body
Rename unused action_type to _action_type
(B007)
750-750: Do not catch blind exception: Exception
(BLE001)
765-765: Do not catch blind exception: Exception
(BLE001)
779-779: Do not catch blind exception: Exception
(BLE001)
780-780: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
807-807: Do not catch blind exception: Exception
(BLE001)
840-840: Do not catch blind exception: Exception
(BLE001)
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
requirements-dev.txt (1)
20-21: Address duplication of psutil and pynvml across requirements files.Lines 20–21 add
psutil>=5.0.0andpynvml>=11.0.0to development dependencies. A prior review flagged that these dependencies are duplicated in bothrequirements.txtandrequirements-dev.txt, creating a maintenance burden.Clarify the intended scope:
- If these are runtime dependencies (as they likely are for the dashboard feature), they should reside only in
requirements.txtand be referenced inrequirements-dev.txtvia-r requirements.txt.- If these are development-only dependencies (e.g., for testing or mocking), they should be removed from
requirements.txtand kept only here.Run the following script to confirm whether these dependencies also exist in
requirements.txt:#!/bin/bash # Check for psutil and pynvml in both requirements files echo "=== Checking requirements.txt ===" rg -n 'psutil|pynvml' requirements.txt || echo "Not found in requirements.txt" echo "" echo "=== Checking requirements-dev.txt ===" rg -n 'psutil|pynvml' requirements-dev.txt || echo "Not found in requirements-dev.txt"
1 participant
Summary
cortex installcommand to install any packages as intended.benchanddoctorcommand.Type of Change
Checklist
Testing
run following command -
cortex dashboardto test as user. or run Pytest to test everything or specify test_dashboard.pyVideo
https://drive.google.com/file/d/1jTAn2CdTlGjoLdYTdKKVpSEvrVD1qwSj/view?usp=sharing
note
This is a good feature but is slow, it can be optimized with less GUI and UX but for now it works
Summary by CodeRabbit
New Features
Documentation
Tests
Dependencies
✏️ Tip: You can customize this high-level summary in your review settings.