Update mcp-agent apps list UI to use lists instead of tables #408
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughReplaced Rich table-based rendering with plain, line-oriented text output for app listings and configurations. Added explicit empty-state headers and separators. Updated _server_status_text to return color-marked strings. Docstrings revised. No public API changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/mcp_agent/cli/cloud/commands/apps/list/main.py (3)
74-88: Use dynamic separator instead of fixed-width lineA fixed 80-char rule may wrap on narrow terminals. Prefer a console-native rule to adapt to terminal width.
- console.print("\n" + "─" * 80 + "\n") + console.rule()
137-169: Prefer “Unknown” over “Offline” when appServerInfo is missingKeeps semantics accurate and consistent with other commands.
- status = _server_status_text( - config.appServerInfo.status - if config.appServerInfo - else "APP_SERVER_STATUS_OFFLINE" - ) + status = _server_status_text( + config.appServerInfo.status if config.appServerInfo else None + )
171-178: Tighten _server_status_text signature; remove unused arg; clarify docstring
- is_last_row is unused; drop it.
- Accept Optional[str] to support “Unknown” fallback.
- Docstring should mention color-marked output.
-def _server_status_text(status: str, is_last_row: bool = False): - """Convert server status code to emoji.""" +def _server_status_text(status: Optional[str]) -> str: + """Convert server status code to color-marked text with emoji for Rich console.""" if status == "APP_SERVER_STATUS_ONLINE": - return "[green]🟢 Online[/green]" + return "[green]🟢 Online[/green]" elif status == "APP_SERVER_STATUS_OFFLINE": - return "[red]🔴 Offline[/red]" + return "[red]🔴 Offline[/red]" else: return "❓ Unknown"Follow-up (optional): this helper exists in another command (app/status). Consider centralizing to a shared utility to avoid divergence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/mcp_agent/cli/cloud/commands/apps/list/main.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/mcp_agent/cli/cloud/commands/apps/list/main.py (3)
src/mcp_agent/cli/utils/ux.py (1)
print_info(32-49)src/mcp_agent/cli/cloud/commands/app/status/main.py (1)
_server_status_text(114-120)src/mcp_agent/cli/mcp_app/api_client.py (1)
MCPAppConfiguration(33-38)
Comment on lines
+111
to
+133
| """Print a list of deployed apps in a clean, copyable format.""" | ||
| console.print(f"\n[bold blue]📦 Deployed MCP Apps ({len(apps)})[/bold blue]") | ||
|
|
||
| table.add_column("Name", style="cyan", overflow="fold") | ||
| table.add_column("ID", style="bright_blue", no_wrap=True) | ||
| table.add_column("Description", style="cyan", overflow="fold") | ||
| table.add_column("Server URL", style="bright_blue", no_wrap=True) | ||
| table.add_column("Status", style="bright_blue", no_wrap=True) | ||
| table.add_column("Created", style="cyan", overflow="fold") | ||
|
|
||
| for idx, app in enumerate(apps): | ||
| is_last_row = idx == len(apps) - 1 | ||
| table.add_row( | ||
| app.name, | ||
| app.appId, | ||
| app.description, | ||
| app.appServerInfo.serverUrl if app.appServerInfo else "", | ||
| _server_status_text( | ||
| ( | ||
| app.appServerInfo.status | ||
| if app.appServerInfo | ||
| else "APP_SERVER_STATUS_OFFLINE" | ||
| ), | ||
| is_last_row, | ||
| ), | ||
| app.createdAt.strftime("%Y-%m-%d %H:%M:%S"), | ||
| for i, app in enumerate(apps): | ||
| if i > 0: | ||
| console.print() | ||
|
|
||
| status = _server_status_text( | ||
| app.appServerInfo.status | ||
| if app.appServerInfo | ||
| else "APP_SERVER_STATUS_OFFLINE" | ||
| ) | ||
|
|
||
| console.print(table) | ||
| console.print(f"[bold cyan]{app.name or 'Unnamed'}[/bold cyan] {status}") | ||
| console.print(f" App ID: {app.appId}") | ||
|
|
||
| if app.appServerInfo and app.appServerInfo.serverUrl: | ||
| console.print(f" Server: {app.appServerInfo.serverUrl}") | ||
|
|
||
| if app.description: | ||
| console.print(f" Description: {app.description}") | ||
|
|
||
| console.print(f" Created: {app.createdAt.strftime('%Y-%m-%d %H:%M:%S')}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against None createdAt and don’t default missing server info to “Offline”
- createdAt may be None (seen on configurations; apps can be similar) → AttributeError risk.
- Treat missing appServerInfo as Unknown, not Offline.
- status = _server_status_text(
- app.appServerInfo.status
- if app.appServerInfo
- else "APP_SERVER_STATUS_OFFLINE"
- )
+ status = _server_status_text(
+ app.appServerInfo.status if app.appServerInfo else None
+ )
@@
- console.print(f" Created: {app.createdAt.strftime('%Y-%m-%d %H:%M:%S')}")
+ if app.createdAt:
+ console.print(f" Created: {app.createdAt.strftime('%Y-%m-%d %H:%M:%S')}")
+ else:
+ console.print(" Created: -")Additionally update the helper to accept Optional status (see suggestion below).
📝 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.
Suggested change
| """Print a list of deployed apps in a clean, copyable format.""" | |
| console.print(f"\n[bold blue]📦 Deployed MCP Apps ({len(apps)})[/bold blue]") | |
| table.add_column("Name", style="cyan", overflow="fold") | |
| table.add_column("ID", style="bright_blue", no_wrap=True) | |
| table.add_column("Description", style="cyan", overflow="fold") | |
| table.add_column("Server URL", style="bright_blue", no_wrap=True) | |
| table.add_column("Status", style="bright_blue", no_wrap=True) | |
| table.add_column("Created", style="cyan", overflow="fold") | |
| for idx, app in enumerate(apps): | |
| is_last_row = idx == len(apps) - 1 | |
| table.add_row( | |
| app.name, | |
| app.appId, | |
| app.description, | |
| app.appServerInfo.serverUrl if app.appServerInfo else "", | |
| _server_status_text( | |
| ( | |
| app.appServerInfo.status | |
| if app.appServerInfo | |
| else "APP_SERVER_STATUS_OFFLINE" | |
| ), | |
| is_last_row, | |
| ), | |
| app.createdAt.strftime("%Y-%m-%d %H:%M:%S"), | |
| for i, app in enumerate(apps): | |
| if i > 0: | |
| console.print() | |
| status = _server_status_text( | |
| app.appServerInfo.status | |
| if app.appServerInfo | |
| else "APP_SERVER_STATUS_OFFLINE" | |
| ) | |
| console.print(table) | |
| console.print(f"[bold cyan]{app.name or 'Unnamed'}[/bold cyan] {status}") | |
| console.print(f" App ID: {app.appId}") | |
| if app.appServerInfo and app.appServerInfo.serverUrl: | |
| console.print(f" Server: {app.appServerInfo.serverUrl}") | |
| if app.description: | |
| console.print(f" Description: {app.description}") | |
| console.print(f" Created: {app.createdAt.strftime('%Y-%m-%d %H:%M:%S')}") | |
| """Print a list of deployed apps in a clean, copyable format.""" | |
| console.print(f"\n[bold blue]📦 Deployed MCP Apps ({len(apps)})[/bold blue]") | |
| for i, app in enumerate(apps): | |
| if i > 0: | |
| console.print() | |
| status = _server_status_text( | |
| app.appServerInfo.status if app.appServerInfo else None | |
| ) | |
| console.print(f"[bold cyan]{app.name or 'Unnamed'}[/bold cyan] {status}") | |
| console.print(f" App ID: {app.appId}") | |
| if app.appServerInfo and app.appServerInfo.serverUrl: | |
| console.print(f" Server: {app.appServerInfo.serverUrl}") | |
| if app.description: | |
| console.print(f" Description: {app.description}") | |
| if app.createdAt: | |
| console.print(f" Created: {app.createdAt.strftime('%Y-%m-%d %H:%M:%S')}") | |
| else: | |
| console.print(" Created: -") |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The existing
mcp-agent apps listcommand is pretty unusable, especially on smaller CLI windows:Let's just list the apps and configured apps directly so that IDs and server URLs can be easily copied
Summary by CodeRabbit
New Features
Style