feat: migrate interactive mode to Terminal.Gui TUI framework#242
feat: migrate interactive mode to Terminal.Gui TUI framework#242joshsmithxrm merged 3 commits intomainfrom
Conversation
Summary of ChangesHello @joshsmithxrm, 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 undertakes a significant refactoring of the interactive mode within the PPDS CLI by transitioning its underlying UI framework to Terminal.Gui. This foundational change not only enhances the interactive user experience but also establishes a clear architectural separation between UI components and core application services. It introduces a functional interactive SQL query interface and formalizes a multi-UI platform strategy through new Architecture Decision Records, promoting consistency and reusability across diverse frontends like the CLI, TUI, and a planned VS Code extension. 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
|
There was a problem hiding this comment.
Code Review
This pull request is a major feature contribution, migrating the interactive mode to the Terminal.Gui framework. The changes are extensive, including the new TUI application structure, supporting views and screens, new architectural decision records (ADRs), and unit tests. The architecture with a multi-interface platform in mind is well-thought-out and documented in the ADRs.
My review focuses on the new TUI implementation. I've identified several high-severity issues related to asynchronous programming and UI threading best practices that should be addressed:
- Cancellation: The
CancellationTokenfrom the command host is ignored, making async operations during startup and execution un-cancellable, aligning with the rule on integrating cancellation tokens. - Threading: UI elements are updated from background threads without marshalling to the main UI loop, which can cause instability. Also, a fire-and-forget async call in a constructor can lead to race conditions.
- Concurrency: The 'load more' functionality is susceptible to race conditions from concurrent executions.
- Disposal: There are sync-over-async calls during disposal which can lead to deadlocks.
I've also included a medium-severity comment about ensuring culture-invariant formatting for numeric values. Addressing these points will significantly improve the robustness and stability of the new TUI.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR migrates the interactive mode from Spectre.Console to Terminal.Gui 1.19, establishing PPDS as a multi-interface platform with comprehensive TUI capabilities. The change introduces three new ADRs (0024-0026) that define the architecture for supporting multiple UIs (CLI, TUI, VS Code extension) with shared services and local state management.
Key changes:
- TUI framework migration: Replaces Spectre.Console interactive mode with Terminal.Gui full-screen application
- Architecture documentation: Three ADRs establish patterns for shared state (ADR-0024), progress reporting (ADR-0025), and error handling (ADR-0026)
- Test coverage: 23 new unit tests for QueryResultConverter value formatting logic
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/PPDS.Cli/Tui/PpdsApplication.cs |
Entry point for Terminal.Gui TUI with InteractiveSession lifecycle management |
src/PPDS.Cli/Tui/MainWindow.cs |
Main menu window with navigation to SQL Query and other planned features |
src/PPDS.Cli/Tui/Screens/SqlQueryScreen.cs |
SQL query screen with ISqlQueryService integration, pagination, and keyboard shortcuts |
src/PPDS.Cli/Tui/Views/QueryResultsTableView.cs |
TableView wrapper with pagination, clipboard copy, and browser integration |
src/PPDS.Cli/Tui/Views/QueryResultConverter.cs |
Utility for converting QueryResult to DataTable with value formatting |
tests/PPDS.Cli.Tests/Tui/Views/QueryResultConverterTests.cs |
Comprehensive unit tests for QueryResultConverter (23 tests) |
src/PPDS.Cli/Commands/InteractiveCommand.cs |
Updated to launch PpdsApplication instead of previous interactive CLI |
src/PPDS.Cli/PPDS.Cli.csproj |
Adds Terminal.Gui 1.19 package reference |
docs/adr/0024_SHARED_LOCAL_STATE.md |
ADR defining ~/.ppds/ as single source of truth for all UIs |
docs/adr/0025_UI_AGNOSTIC_PROGRESS.md |
ADR for IProgressReporter pattern for long operations |
docs/adr/0026_STRUCTURED_ERROR_MODEL.md |
ADR for PpdsException hierarchy with ErrorCode/UserMessage |
CLAUDE.md |
Updated with multi-UI architecture guidance, ADR references, and new rules |
.claude/design.md |
New design document for TUI enhancements roadmap |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Bot Review ResponseAddressed the following findings in commit 82e4cdb: Gemini Findings (Fixed)
Code Scanning Findings (Fixed)
|
82e4cdb to
5a9d719
Compare
- Add Terminal.Gui 1.19 NuGet package for TUI rendering - Create TUI application structure (PpdsApplication, MainWindow, SqlQueryScreen) - Create QueryResultsTableView with pagination, copy, and URL features - Extract QueryResultConverter for testable value formatting logic - Update InteractiveCommand to launch Terminal.Gui app Architecture documentation (ADRs): - ADR-0024: Shared Local State Architecture (~/.ppds/ for all UIs) - ADR-0025: UI-Agnostic Progress Reporting (IProgressReporter pattern) - ADR-0026: Structured Error Model (PpdsException hierarchy) Updates CLAUDE.md with Platform Architecture section explaining the multi-interface platform (CLI, TUI, VS Code extension). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Pass cancellationToken from InteractiveCommand to PpdsApplication - Register cancellation token to stop Terminal.Gui application - Add proper error handling for fire-and-forget async in constructor - Use Application.MainLoop.Invoke() for UI updates from async methods - Add guard flag to prevent concurrent LoadMoreAsync calls - Replace generic catch clauses with specific exceptions (InvalidOperationException, HttpRequestException) - Add documentation comment explaining sync-over-async in Dispose 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
5a9d719 to
dbf1972
Compare
Summary
Architecture Changes
This PR establishes PPDS as a multi-interface platform where:
ppds -i) uses Terminal.Gui for full application experienceppds serveRPCNew ADRs
~/.ppds/via Application ServicesIProgressReporterpattern for long operationsPpdsExceptionhierarchy with ErrorCode/UserMessageNew Files
Test plan
ppds -ilaunches Terminal.Gui TUIRelated Issues
Addresses issues #204, #205, #206, #207, #208, #234 (TUI enhancements epic)
🤖 Generated with Claude Code