Skip to content

Commit 78c06b1

Browse files
joshsmithxrmclaude
andcommitted
fix(tui): address code review findings for multi-tab architecture
- Remove global EnvironmentChanged subscription from SqlQueryScreen (I-2) - Make service caching thread-safe with Lazy<T> in InteractiveSession (I-1) - Consolidate dual activation path: NavigateTo delegates to OnActiveTabChanged (I-3) - Add Alt+1-9 hotkeys for direct tab selection (M-4) - Assert spinner state transitions in SplashView tests (M-6) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent c7f1300 commit 78c06b1

File tree

4 files changed

+39
-69
lines changed

4 files changed

+39
-69
lines changed

src/PPDS.Cli/Tui/InteractiveSession.cs

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,13 @@ internal sealed class InteractiveSession : IAsyncDisposable
4545
private string? _activeEnvironmentUrl;
4646
private string? _activeEnvironmentDisplayName;
4747
private bool _disposed;
48-
private ITuiErrorService? _errorService;
49-
private IHotkeyRegistry? _hotkeyRegistry;
50-
private IProfileService? _profileService;
51-
private IEnvironmentService? _environmentService;
52-
private ITuiThemeService? _themeService;
53-
private IQueryHistoryService? _queryHistoryService;
54-
private IExportService? _exportService;
48+
private readonly Lazy<ITuiErrorService> _errorService;
49+
private readonly Lazy<IHotkeyRegistry> _hotkeyRegistry;
50+
private readonly Lazy<IProfileService> _profileService;
51+
private readonly Lazy<IEnvironmentService> _environmentService;
52+
private readonly Lazy<ITuiThemeService> _themeService;
53+
private readonly Lazy<IQueryHistoryService> _queryHistoryService;
54+
private readonly Lazy<IExportService> _exportService;
5555

5656
/// <summary>
5757
/// Event raised when the environment changes (either via initialization or explicit switch).
@@ -104,6 +104,15 @@ public InteractiveSession(
104104
_serviceProviderFactory = serviceProviderFactory ?? new ProfileBasedServiceProviderFactory();
105105
_deviceCodeCallback = deviceCodeCallback;
106106
_beforeInteractiveAuth = beforeInteractiveAuth;
107+
108+
// Initialize lazy service instances (thread-safe by default)
109+
_profileService = new Lazy<IProfileService>(() => new ProfileService(_profileStore, NullLogger<ProfileService>.Instance));
110+
_environmentService = new Lazy<IEnvironmentService>(() => new EnvironmentService(_profileStore, NullLogger<EnvironmentService>.Instance));
111+
_themeService = new Lazy<ITuiThemeService>(() => new TuiThemeService());
112+
_errorService = new Lazy<ITuiErrorService>(() => new TuiErrorService());
113+
_hotkeyRegistry = new Lazy<IHotkeyRegistry>(() => new HotkeyRegistry());
114+
_queryHistoryService = new Lazy<IQueryHistoryService>(() => new QueryHistoryService(NullLogger<QueryHistoryService>.Instance));
115+
_exportService = new Lazy<IExportService>(() => new ExportService(NullLogger<ExportService>.Instance));
107116
}
108117

109118
/// <summary>
@@ -452,7 +461,7 @@ await profileService.SetEnvironmentAsync(profileName, environmentUrl, environmen
452461
/// <returns>The profile service.</returns>
453462
public IProfileService GetProfileService()
454463
{
455-
return _profileService ??= new ProfileService(_profileStore, NullLogger<ProfileService>.Instance);
464+
return _profileService.Value;
456465
}
457466

458467
/// <summary>
@@ -462,7 +471,7 @@ public IProfileService GetProfileService()
462471
/// <returns>The environment service.</returns>
463472
public IEnvironmentService GetEnvironmentService()
464473
{
465-
return _environmentService ??= new EnvironmentService(_profileStore, NullLogger<EnvironmentService>.Instance);
474+
return _environmentService.Value;
466475
}
467476

468477
/// <summary>
@@ -481,7 +490,7 @@ public ProfileStore GetProfileStore()
481490
/// <returns>The theme service.</returns>
482491
public ITuiThemeService GetThemeService()
483492
{
484-
return _themeService ??= new TuiThemeService();
493+
return _themeService.Value;
485494
}
486495

487496
/// <summary>
@@ -491,7 +500,7 @@ public ITuiThemeService GetThemeService()
491500
/// <returns>The error service.</returns>
492501
public ITuiErrorService GetErrorService()
493502
{
494-
return _errorService ??= new TuiErrorService();
503+
return _errorService.Value;
495504
}
496505

497506
/// <summary>
@@ -501,7 +510,7 @@ public ITuiErrorService GetErrorService()
501510
/// <returns>The hotkey registry.</returns>
502511
public IHotkeyRegistry GetHotkeyRegistry()
503512
{
504-
return _hotkeyRegistry ??= new HotkeyRegistry();
513+
return _hotkeyRegistry.Value;
505514
}
506515

507516
/// <summary>
@@ -511,7 +520,7 @@ public IHotkeyRegistry GetHotkeyRegistry()
511520
/// <returns>The query history service.</returns>
512521
public IQueryHistoryService GetQueryHistoryService()
513522
{
514-
return _queryHistoryService ??= new QueryHistoryService(NullLogger<QueryHistoryService>.Instance);
523+
return _queryHistoryService.Value;
515524
}
516525

517526
/// <summary>
@@ -521,7 +530,7 @@ public IQueryHistoryService GetQueryHistoryService()
521530
/// <returns>The export service.</returns>
522531
public IExportService GetExportService()
523532
{
524-
return _exportService ??= new ExportService(NullLogger<ExportService>.Instance);
533+
return _exportService.Value;
525534
}
526535

527536
#endregion

src/PPDS.Cli/Tui/Screens/SqlQueryScreen.cs

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -250,9 +250,6 @@ public SqlQueryScreen(Action<DeviceCodeInfo>? deviceCodeCallback, InteractiveSes
250250
_resultsTable.Title = "Results";
251251
};
252252

253-
// Subscribe to environment changes from the session
254-
Session.EnvironmentChanged += OnEnvironmentChanged;
255-
256253
// Initialize results table with environment URL (already set by base constructor)
257254
if (EnvironmentUrl != null)
258255
{
@@ -324,28 +321,6 @@ private void SetupKeyboardHandling()
324321
};
325322
}
326323

327-
private void OnEnvironmentChanged(string? url, string? displayName)
328-
{
329-
Application.MainLoop?.Invoke(() =>
330-
{
331-
if (url != null)
332-
{
333-
EnvironmentUrl = url;
334-
_resultsTable.SetEnvironmentUrl(EnvironmentUrl);
335-
336-
// Clear stale results from previous environment
337-
_resultsTable.ClearData();
338-
_lastSql = null;
339-
_lastPagingCookie = null;
340-
_lastPageNumber = 1;
341-
}
342-
else
343-
{
344-
EnvironmentUrl = null;
345-
}
346-
});
347-
}
348-
349324
private async Task ExecuteQueryAsync()
350325
{
351326
var sql = _queryInput.Text.ToString() ?? string.Empty;
@@ -782,6 +757,5 @@ public SqlQueryScreenState CaptureState()
782757

783758
protected override void OnDispose()
784759
{
785-
Session.EnvironmentChanged -= OnEnvironmentChanged;
786760
}
787761
}

src/PPDS.Cli/Tui/TuiShell.cs

Lines changed: 14 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -100,40 +100,18 @@ public TuiShell(string? profileName, Action<DeviceCodeInfo>? deviceCodeCallback,
100100

101101
/// <summary>
102102
/// Opens a screen in a new tab and makes it active.
103+
/// Activation is handled by <see cref="OnActiveTabChanged"/> via the TabManager event.
103104
/// </summary>
104105
public void NavigateTo(ITuiScreen screen)
105106
{
106107
// Clear splash/main menu if still showing
107108
ClearSplashAndMainMenu();
108109

109-
// Deactivate current screen (if any)
110-
if (_currentScreen != null)
111-
{
112-
_currentScreen.CloseRequested -= OnScreenCloseRequested;
113-
_currentScreen.MenuStateChanged -= OnScreenMenuStateChanged;
114-
_currentScreen.OnDeactivating();
115-
_contentArea.Remove(_currentScreen.Content);
116-
}
117-
118-
// Add screen as a new tab
110+
// Add screen as a new tab — AddTab fires ActiveTabChanged,
111+
// which calls OnActiveTabChanged to handle activation.
119112
var envUrl = (screen as TuiScreenBase)?.EnvironmentUrl ?? _session.CurrentEnvironmentUrl;
120113
var envName = _session.CurrentEnvironmentDisplayName;
121114
_tabManager.AddTab(screen, envUrl, envName);
122-
123-
// Activate the new screen
124-
_currentScreen = screen;
125-
_currentScreen.CloseRequested += OnScreenCloseRequested;
126-
_currentScreen.MenuStateChanged += OnScreenMenuStateChanged;
127-
_hotkeyRegistry.SetActiveScreen(screen);
128-
_contentArea.Title = screen.Title;
129-
_contentArea.Add(screen.Content);
130-
screen.OnActivated(_hotkeyRegistry);
131-
132-
// Rebuild menu bar with screen-specific items
133-
RebuildMenuBar();
134-
135-
// Set focus to content
136-
screen.Content.SetFocus();
137115
}
138116

139117
private void OnScreenCloseRequested()
@@ -174,10 +152,6 @@ private void OnActiveTabChanged()
174152
{
175153
var activeTab = _tabManager.ActiveTab;
176154

177-
// Skip if already showing the correct screen (NavigateTo already activated it)
178-
if (activeTab != null && _currentScreen == activeTab.Screen)
179-
return;
180-
181155
// Deactivate current screen if any
182156
if (_currentScreen != null)
183157
{
@@ -451,6 +425,17 @@ private void RegisterGlobalHotkeys()
451425
HotkeyScope.Global,
452426
"Previous tab",
453427
() => _tabManager.ActivatePrevious()));
428+
429+
// Direct tab selection: Alt+1 through Alt+9
430+
for (var i = 0; i < 9; i++)
431+
{
432+
var index = i;
433+
_hotkeyRegistrations.Add(_hotkeyRegistry.Register(
434+
Key.AltMask | (Key)('1' + i),
435+
HotkeyScope.Global,
436+
$"Tab {i + 1}",
437+
() => _tabManager.ActivateTab(index)));
438+
}
454439
}
455440

456441
private void NavigateToSqlQuery()

tests/PPDS.Cli.Tests/Tui/Views/SplashViewTests.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ public void InitialState_ShowsConnecting()
1313
var state = splash.CaptureState();
1414

1515
Assert.False(state.IsReady);
16+
Assert.True(state.SpinnerActive);
1617
Assert.NotNull(state.Version);
1718
Assert.NotEmpty(state.StatusMessage);
1819
}
@@ -38,6 +39,7 @@ public void SetReady_MarksReady()
3839
var state = splash.CaptureState();
3940

4041
Assert.True(state.IsReady);
42+
Assert.False(state.SpinnerActive);
4143
}
4244

4345
[Fact]

0 commit comments

Comments
 (0)