Refactor PTUI dashboard into modular package #3
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the monolithic ptui_dashboard.py script into a modular package structure while maintaining backward compatibility through a thin compatibility shim. The refactoring improves code organization, testability, and maintainability by splitting concerns into focused modules.
Key Changes:
- Created
ptui_dashboard_corepackage with 8 specialized modules (models, config, network, monitor, actions, ui, utils, init) - Rewrote
scripts/ptui_dashboard.pyas a 200-line compatibility shim that re-exports the core package's API - Updated documentation to reflect the new modular architecture and adjusted the comparison table
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/ptui_dashboard_core/__init__.py |
Public API surface with comprehensive re-exports from all submodules |
scripts/ptui_dashboard_core/models.py |
Dataclass definitions for Service, ActionItem, and MenuItem |
scripts/ptui_dashboard_core/config.py |
Environment variable validation and YAML-based service configuration loader |
scripts/ptui_dashboard_core/network.py |
Synchronous and asynchronous HTTP JSON fetching with optional aiohttp support |
scripts/ptui_dashboard_core/monitor.py |
Service health checking and state gathering logic with async/sync modes |
scripts/ptui_dashboard_core/actions.py |
Quick action handlers including refresh, health probe, and validation runner |
scripts/ptui_dashboard_core/ui.py |
Complete curses UI implementation with rendering, event handling, and terminal setup |
scripts/ptui_dashboard_core/utils.py |
Shared utility functions like latency formatting |
scripts/ptui_dashboard.py |
Compatibility shim providing historical API surface while delegating to core package |
docs/ptui-dashboard.md |
Updated documentation describing modular architecture and extension points |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| message = f"Health probe: failing services - {', '.join(failing)}." | ||
| else: | ||
| message = "Health probe completed." | ||
| message = f"Health probe: failing services - {', '.join(failing)}." |
There was a problem hiding this comment.
The logic for handling failures is missing the else clause that existed in the old code. When there are failing services, this code now always returns the failing services message, but the old code would return "Health probe completed." if the failing list is empty. This appears to be a logic change that could affect behavior.
| message = f"Health probe: failing services - {', '.join(failing)}." | |
| if not failing: | |
| message = "Health probe completed." | |
| else: | |
| message = f"Health probe: failing services - {', '.join(failing)}." |
| def gather_state_smart(timeout: float) -> dict[str, Any]: | ||
| """Gather state using async if available, otherwise fallback to sync. | ||
| This is the main entry point that other code should use. | ||
| """ | ||
| if ASYNC_AVAILABLE: | ||
| # Use async version for better performance | ||
| return asyncio.run(gather_state_async(timeout)) | ||
|
|
||
| # Fallback to synchronous version | ||
| _sync_services() | ||
| return _monitor.gather_state_smart(timeout) | ||
| return gather_state(timeout) |
There was a problem hiding this comment.
When ASYNC_AVAILABLE is True, this function calls _monitor.gather_state_smart(timeout) which will internally check ASYNC_AVAILABLE again and call asyncio.run(gather_state_async(timeout)). However, gather_state_async is defined locally in this file (lines 104-106) and it calls _monitor.gather_state_async(timeout), but the local SERVICES list may have been modified and needs to be synchronized via _sync_services() first. The _sync_services() call on line 111 is placed, but when calling _monitor.gather_state_smart() on line 112, the flow will go through the core module's version which doesn't use the locally modified SERVICES. Consider simplifying this logic.
| def main() -> int: | ||
| """Launch the curses dashboard.""" | ||
|
|
||
| return run_dashboard() |
There was a problem hiding this comment.
[nitpick] The main() function signature changed from returning None (with implicit sys.exit() calls) to returning int. While this is a cleaner approach, tests or external code that expect main() to return None may break. The change appears intentional for the refactor, but ensure existing tests and callers are updated accordingly.
| "run_validation", | ||
| "safe_addstr", | ||
| "subprocess", | ||
| "urlopen", |
There was a problem hiding this comment.
[nitpick] The urlopen function is exported in __all__ but it's only imported, not defined or re-exported explicitly. While Python allows this, it's unusual to export imported standard library functions unless they're part of the module's public API. Consider whether this is intentionally part of the backward compatibility surface or if it should be removed.
| "urlopen", |
| else: | ||
| display_name = provider_data.get("description", key) | ||
| endpoint = provider_data.get("health_endpoint", "/health") |
There was a problem hiding this comment.
[nitpick] The health endpoint default changed from "/health" (in the old code) to provider_data.get("health_endpoint", "/health") which is correct. However, this is a subtle change in behavior - the old code didn't specify a default from the provider_data, it just used "/health". The new code is more flexible and correct, but ensure this doesn't break existing configurations that relied on the old behavior.
|
Heads-up: dashboard direction is now the Hello Kitty Textual app (see scripts/ai-dashboard and dashboards/hello_kitty). Please align this PTUI refactor with that plan or discuss whether PTUI remains as secondary/read-only. Suggest pausing merge until we confirm scope against the Hello Kitty dashboard. |
Alignment checklist for Hello Kitty–only dashboard direction:\n- Launcher must remain → ; please remove/deprecate any WTH/Bubble Tea paths.\n- If PTUI stays, mark it secondary/SSH-only and add a quick smoke () plus note its scope.\n- Validation: run ================================================================================
|
|
Alignment checklist for the Hello Kitty-only dashboard direction:
|
Summary
ptui_dashboard_corepackage that splits the curses dashboard into focused modules for models, config, network helpers, monitoring, actions, utilities, and UI renderingscripts/ptui_dashboard.pyinto a thin compatibility shim that keeps the historical API surface (for tests and CLI tooling) while delegating to the new package and refreshing validation logicTesting
Codex Task