fix(tui): tab contrast, escape behavior, and dispose guard #524
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
…nsition Override the terminal's 16-color ANSI palette via OSC 4 escape sequences on startup so the TUI renders identically regardless of terminal color theme. Restore the original palette on exit via OSC 104. Respects NO_COLOR and redirected output. Wire InitializeAsync completion to the splash screen so it transitions from "Initializing..." → "Ready" → main menu instead of spinning forever. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The AcquireTokenByUsernamePassword API is deprecated but still functional. PPDS intentionally supports username/password auth for environments where interactive auth isn't viable. Added pragma with migration link for future reference. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ADRs have been superseded by specs. Remove the docs/adr/ directory and all references to ADR documents across markdown, source code comments, and config files. Update pointers to use docs/specs/. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Three fixes for multi-environment tab support: 1. SqlQueryScreen.Title now uses captured EnvironmentDisplayName instead of Session.CurrentEnvironmentDisplayName, so each tab shows its own environment rather than the global session state. 2. OnActiveTabChanged now calls UpdateDisplayedEnvironment to sync the status bar with the active tab's environment without persisting to profile or pre-warming providers. 3. Tab labels now include environment badge (e.g., [DEV], [PROD]) and use environment-tinted colors (red=PROD, yellow=SANDBOX, green=DEV) for inactive tabs to visually distinguish environments. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cover UpdateDisplayedEnvironment, environment capture at screen creation, SqlQueryScreen title formatting, GetTabScheme color logic, and TabBar environment type detection from commit 1d9ed05. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The debounce handler on MenuBar.MouseClick fired before MenuBar's internal MouseEvent() toggle logic, randomly suppressing clicks the framework needed to maintain open/close state. This is a known Terminal.Gui v1.x WindowsDriver bug (issues #386, #1848) — fixed in v2. Added comment documenting the constraint. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
TabBar was positioned at Y=0 overlapping the MenuBar, making it invisible. Moved to Y=1 so it renders between menu and content area. KeyboardShortcutsDialog now reads live bindings from IHotkeyRegistry instead of a hardcoded list, eliminating divergence between registered hotkeys and what the help dialog displays. Tab management shortcuts (Ctrl+T, Ctrl+W, Ctrl+Tab, etc.) were registered but never shown. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add persistent environment configuration support with: - EnvironmentColor enum (13 terminal palette colors) - EnvironmentConfig model (url, label, type, color) - EnvironmentConfigCollection (root object for environments.json) - EnvironmentConfigStore (load/save/get/remove with file-based persistence) - ProfilePaths additions for environments.json path Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rehensive unit tests Introduces the application service layer for environment configuration: - IEnvironmentConfigService interface with color/type/label resolution - EnvironmentConfigService with built-in type defaults, URL heuristics, and priority-based resolution (explicit > type-default > URL > gray) - 9 unit tests for EnvironmentConfigStore (CRUD, merge, normalize, round-trip) - 16 unit tests for EnvironmentConfigService (resolution priority, type defaults) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…se tabs Environment selector now opens a new SQL Query tab on the selected environment (or switches to an existing tab if one is already open on that URL). Previously it only updated the session state while the active tab kept querying its original environment. Profile switch closes all open tabs since connection pools are invalidated when credentials change. Returns to main menu. Added TabManager.FindTabByEnvironment() for tab-by-URL lookup and CloseAllTabs() for bulk close. SqlQueryScreen constructor now accepts explicit environmentUrl and displayName for targeted tab creation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…-configurable colors Add EnvironmentColor-based overloads to TuiColorPalette for status bar and tab schemes. Update ITuiThemeService with config-aware methods (GetStatusBarSchemeForUrl, GetEnvironmentLabelForUrl, GetResolvedColor). Update TuiStatusBar, TabManager, and TabBar to use configurable colors instead of hardcoded EnvironmentType detection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…vice into InteractiveSession Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…bel, type, and color Add a new dialog accessible from the environment selector that allows users to configure display settings per environment: custom label, type classification (Production/Sandbox/etc.), and color override. The dialog loads existing config from EnvironmentConfigService and persists changes on save. Also wires a "Configure" button into EnvironmentSelectorDialog. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- TabBar.UpdateHighlight now uses EnvironmentColor instead of EnvironmentType, so user-configured colors persist on tab switch - Config dialog color list adds "(Use type default)" option to avoid accidentally persisting an explicit color override - Shorten type hint text to fit within dialog width Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
TabBar was using a local EnvironmentType-to-string mapping that only knew URL heuristics, causing all tabs to show [PROD] when environments share the same regional datacenter (.crm.dynamics.com). Now uses ITuiThemeService.GetEnvironmentLabelForUrl() which goes through the full resolution chain: user config → Discovery API type → URL heuristics. This matches the status bar's label resolution. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The CRM URL suffix (crm, crm2, crm4, crm9, etc.) indicates geographic region (NA, SA, EMEA, UK), not environment type. A production org can be on crm4 and a sandbox on crm. Removed the Sandbox/Production heuristics based on these suffixes from both TuiThemeService and EnvironmentConfigService. Keyword detection (dev, test, qa, trial in org name) is retained as a last-resort hint. The correct source is the Discovery API type stored in the profile, or explicit user configuration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Wire UI refresh after EnvironmentConfigDialog saves (ConfigChanged event on InteractiveSession, RefreshTabColors on TabManager, handler in TuiShell) - Fix GetEnvironmentLabelForUrl to check ResolveLabelAsync before type abbreviation, so user-configured custom labels are displayed - Add tab close confirmation when screen has unsaved work (HasUnsavedWork on ITuiScreen/TuiScreenBase, MessageBox in NavigateBack) - Add state capture to EnvironmentConfigDialog (ITuiStateCapture pattern) - Remove "Coming Soon" menu items and main menu button - Add tests for RefreshTabColors and ConfigChanged event (173 → 179 tests) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…iew panel, and config access Environment selector now shows resolved type labels from config service instead of raw Discovery API types. Added preview panel showing URL, type, label, color, and config status for selected environment. Configure Environment is now accessible from Tools menu and status bar right-click, not just the selector dialog. Selecting an unconfigured environment prompts to set label/type/color with Discovery API type pre-seeded. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…utton Add two missing auth methods to ProfileCreationDialog: - Certificate Store (Windows-only, thumbprint-based SPN auth) - Username & Password (ROPC flow with MFA warning) Dialog redesigned with platform-aware method list, semantic AuthMethod mapping instead of index-based logic, relative positioning via Pos.Bottom(), and separate Credentials frame for username/password. Add explicit Rename button to ProfileSelectorDialog row 1, replacing the hidden F2-only keyboard shortcut with a discoverable UI element. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Terminal.Gui's Application.Init() sets Colors.Base/TopLevel/Menu/Dialog/Error to its own built-in theme. Views that fall back to these globals rendered with terminal default colors on the first frame, only correcting after an environment switch triggered a redraw. Override all five globals with our TuiColorPalette immediately after Init() so the first render uses our dark theme. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Two issues caused wrong colors on first load after splash: 1. TuiTerminalPalette.Apply() ran before Application.Init(), so the OSC 4 palette override was applied to the wrong screen buffer. Moved to after Init so it affects the buffer Terminal.Gui actually renders on. 2. Application.Top.ColorScheme was set by Init() to the old Colors.TopLevel default. Overriding Colors.TopLevel afterward doesn't retroactively update Top's existing reference. Now explicitly set Top.ColorScheme. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…plash Environment selector was unconditionally opening a SQL tab when no matching tab existed, even when the user was on the splash screen with no tabs open. Now only performs tab navigation when tabs are already open, letting the user stay on splash and choose what to open next. Also removes the old main menu transition — splash now stays as the persistent home screen instead of auto-transitioning after init. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Subclass MenuBar to fix two flicker issues at the correct event channel: - Alt key toggle: bare Alt can only open menus, not close them (matches VS Code/Windows Terminal behavior). Closes via Esc/click instead. - Mouse click debounce: suppress close events within 300ms of open to prevent Windows console driver duplicate event bug. Consolidate all menu reflection (openedByHotKey, CloseAllMenus) into PpdsMenuBar. Remove SuppressNextAltMenuFocus hack from HotkeyRegistry and SqlQueryScreen. HotkeyRegistry now uses Func<bool> for menu-open check instead of holding a MenuBar reference. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…esign Update logo color from BrightGreen to BrightCyan for visual consistency. Add design doc for smart copy/paste redesign with column-scoped selection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Redesign copy behavior: Ctrl+C copies single cell value or multi-cell selection with headers (TSV). Ctrl+Shift+C inverts header behavior. Selection respects column bounds from MultiSelectedRegions. Status bar shows self-teaching hints about the alternative shortcut. Extract shared TableCopyHelper used by both QueryResultsTableView and DataTableView, replacing duplicated copy logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comprehensive spec for evolving the SQL query engine with execution plan layer (Volcano model), expression evaluator, TDS Endpoint acceleration, parallel aggregate partitioning, DML via SQL, metadata schema, and window functions. Includes 25-task implementation plan with dependency graph for parallel agent execution. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Status bar now shows context-aware copy shortcuts: single-cell selection shows "copy value / with header", multi-cell shows "copy with headers / values only". Updates live as selection changes via SelectedCellChanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Foundation for query engine v2 execution plan layer. Introduces expression tree types (binary, unary, function, case, cast, aggregate) and statement type hierarchy. No behavioral changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
joshsmithxrm
temporarily deployed
to
test-dataverse
GitHub Actions
Inactive
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
- Separate test/qa/uat URLs into DetectEnvironmentType_TestKeywords_ReturnsTest (was incorrectly grouped under DevKeywords expecting Development) - Add missing EnvironmentType.Test case to GetEnvironmentLabel test - Update E2E navigateToSqlQuery helper for expanded frame title with Ctrl+Space, Alt+arrows hints - Update Escape test to verify tab stays open (new behavior) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
joshsmithxrm
temporarily deployed
to
test-dataverse
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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 significantly enhances the TUI (Text User Interface) experience by refining visual elements, improving user interaction, and addressing a critical stability bug. The changes introduce dynamic tab coloring for better contrast and readability, ensuring active environments are clearly distinguishable. User workflow is streamlined by modifying the Escape key's behavior in SQL Query tabs, preventing unintended tab closures and promoting more deliberate actions. Furthermore, a robust fix for an Highlights
Changelog
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
|
Replace `as` casts followed by `.Should().NotBeNull()` and null-forgiving operator with direct casts across 10 test files. Direct casts throw InvalidCastException on failure (equivalent test behavior) and eliminate 185 cs/dereferenced-value-may-be-null CodeQL alerts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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 very large and combines many unrelated changes, including major architectural refactoring of the query engine, documentation cleanup, and various TUI fixes and features. The title and description are misleading and only cover a small fraction of the changes. This makes the PR very difficult to review and violates the principle of small, focused pull requests. I'll recommend splitting it into multiple smaller PRs. My review will focus on the most significant issues found within the provided code patches.
SqlParser.Parse() already returns SqlSelectStatement, making the explicit casts unnecessary. Resolves 12 CodeQL useless-cast-to-self alerts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The (object?)null casts are unnecessary since the tuple element type is already object?, so null is implicitly typed correctly. Resolves 12 CodeQL useless-upcast alerts across 5 test files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolve CodeQL cs/useless-assignment-to-local alerts by removing or discarding variables that are assigned but never read: - TuiShell: remove unused `active` and `profile` locals in profile refresh methods - SyntaxHighlightedTextView: use discard for fire-and-forget async, remove unused `subFrame` - EnvironmentDetailsDialog: remove unused `region` variable - TdsQueryExecutor: declare `columns` without initializing to empty list (reassigned before use) - Test files: use `var _` discard in `await foreach` loops where row is not consumed Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Convert manual Dispose() calls to using-var declarations so disposable objects are deterministically cleaned up even when assertions throw before the explicit Dispose() is reached. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add `using` declarations to IDisposable locals that were not being disposed: ContextMenu in DataTableView and CancellationTokenSource in six test files. Resolves CodeQL local-not-disposed alerts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace inefficient ContainsKey followed by indexer lookup with single TryGetValue call in VariableScope and across test assertions. Resolves CodeQL inefficient-containskey alerts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add CodeQL suppression comments for intentional exact float comparisons: - ExpressionEvaluator: divide-by-zero guards comparing double to 0.0 - CastConverter: SQL BIT cast checking float/double against zero These are correct SQL semantics where exact equality is intended. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- DateFunctions: cast int to double before multiplication to prevent precision loss - CachedMetadataProvider: document double-check-lock pattern for CodeQL - EnvironmentConfigService: document nullable coalescing reachability - EnvironmentSelectorDialog: extract complex condition to named booleans - EnvironmentConfigDialog: add missing XML summary on constructor - TuiDialog: suppress virtual-call-in-constructor (Terminal.Gui pattern) - DataTableView: suppress virtual-call-in-constructor (Terminal.Gui pattern) - DataverseConnectionPool: document intentional static field write under lock - UsernamePasswordCredentialProvider: document intentional obsolete API usage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- MetadataScanNodeTests: assert on collected rows list to avoid unused-collection - ClientWindowNodeTests: add Assert.Fail guard so result list is not flagged unused - QueryExecutorTests: assert capturedQueries.Count to validate query execution - ParallelPartitionNodeTests: document intentional static concurrency tracking - EnvironmentServiceTests: catch specific IOException instead of bare catch - ProfileServiceTests: catch specific IOException instead of bare catch Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
joshsmithxrm
had a problem deploying
to
test-dataverse
GitHub Actions
Error
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
joshsmithxrm
temporarily deployed
to
test-dataverse
GitHub Actions
Inactive
tests/PPDS.Dataverse.Tests/Query/Planning/Nodes/ClientWindowNodeTests.cs
Fixed
Show fixed
Hide fixed
tests/PPDS.Dataverse.Tests/Query/Planning/Nodes/ParallelPartitionNodeTests.cs
Dismissed
Show dismissed
Hide dismissed
tests/PPDS.Dataverse.Tests/Query/Planning/Nodes/ParallelPartitionNodeTests.cs
Dismissed
Show dismissed
Hide dismissed
Restructure EnvironmentConfigService null checks to clarify flow for static analysis. Fix test assertions using BeOfType<> instead of as-cast with null-forgiving. Remove unused list variable in cancellation test. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
joshsmithxrm
temporarily deployed
to
test-dataverse
GitHub Actions
Inactive
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
GetContrastingForeground()andGetBrightForegroundColor()helpers._disposedguard toProfileCreationDialog.Dispose()to preventCancellationTokenSource.Cancel()on an already-disposed CTS. Terminal.Gui'sResponder.Dispose()can invokeDispose(true)multiple times.Test plan
🤖 Generated with Claude Code