Skip to content

Commit 82e4cdb

Browse files
joshsmithxrmclaude
joshsmithxrm
and
claude
committed
fix: address bot review comments for TUI implementation
- 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>
1 parent 8f71361 commit 82e4cdb

File tree

4 files changed

+91
-44
lines changed

4 files changed

+91
-44
lines changed

‎src/PPDS.Cli/Commands/InteractiveCommand.cs‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,15 +39,20 @@ public static Command Create()
3939

4040
try
4141
{
42-
var app = new PpdsApplication(
42+
using var app = new PpdsApplication(
4343
profileName: null, // Uses active profile
4444
deviceCodeCallback: ProfileServiceFactory.DefaultDeviceCodeCallback);
4545

46-
return Task.FromResult(app.Run());
46+
return Task.FromResult(app.Run(cancellationToken));
4747
}
48-
catch (Exception ex)
48+
catch (OperationCanceledException)
4949
{
50-
Console.Error.WriteLine($"Interactive mode error: {ex}");
50+
// User cancelled - not an error
51+
return Task.FromResult(ExitCodes.Success);
52+
}
53+
catch (InvalidOperationException ex)
54+
{
55+
Console.Error.WriteLine($"Interactive mode error: {ex.Message}");
5156
AnsiConsole.MarkupLine($"[red]Error: {ex.Message}[/]");
5257
return Task.FromResult(ExitCodes.Failure);
5358
}

‎src/PPDS.Cli/Tui/PpdsApplication.cs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,16 @@ public PpdsApplication(string? profileName, Action<DeviceCodeInfo>? deviceCodeCa
2424
/// <summary>
2525
/// Runs the TUI application. Blocks until the user exits.
2626
/// </summary>
27+
/// <param name="cancellationToken">Token to signal application shutdown.</param>
2728
/// <returns>Exit code (0 for success).</returns>
28-
public int Run()
29+
public int Run(CancellationToken cancellationToken = default)
2930
{
3031
// Create session for connection pool reuse across screens
3132
_session = new InteractiveSession(_profileName, _deviceCodeCallback);
3233

34+
// Register cancellation to request stop
35+
using var registration = cancellationToken.Register(() => Application.RequestStop());
36+
3337
Application.Init();
3438

3539
try
@@ -42,6 +46,9 @@ public int Run()
4246
finally
4347
{
4448
Application.Shutdown();
49+
// Note: Sync-over-async is required here because Terminal.Gui's Application.Run()
50+
// is synchronous and we need to clean up the session before returning.
51+
// The session disposal is fast (just releases pooled connections).
4552
_session?.DisposeAsync().AsTask().GetAwaiter().GetResult();
4653
}
4754
}

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

Lines changed: 50 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using System.Net.Http;
12
using PPDS.Auth.Credentials;
23
using PPDS.Auth.Profiles;
34
using PPDS.Cli.Interactive;
@@ -103,21 +104,31 @@ public SqlQueryScreen(string? profileName, Action<DeviceCodeInfo>? deviceCodeCal
103104

104105
Add(queryFrame, _filterFrame, _resultsTable, _statusLabel);
105106

106-
// Load profile and environment info
107-
_ = LoadProfileInfoAsync();
107+
// Load profile and environment info (fire-and-forget with error handling)
108+
_ = LoadProfileInfoAsync().ContinueWith(t =>
109+
{
110+
if (t.IsFaulted && t.Exception != null)
111+
{
112+
Application.MainLoop?.Invoke(() =>
113+
{
114+
_statusLabel.Text = $"Error loading profile: {t.Exception.InnerException?.Message ?? t.Exception.Message}";
115+
});
116+
}
117+
}, TaskScheduler.Default);
108118

109119
// Set up keyboard shortcuts
110120
SetupKeyboardShortcuts();
111121
}
112122

113123
private async Task LoadProfileInfoAsync()
114124
{
115-
try
116-
{
117-
using var store = new ProfileStore();
118-
var collection = await store.LoadAsync(CancellationToken.None);
119-
var profile = collection.ActiveProfile;
125+
using var store = new ProfileStore();
126+
var collection = await store.LoadAsync(CancellationToken.None);
127+
var profile = collection.ActiveProfile;
120128

129+
// Update UI on main thread
130+
Application.MainLoop?.Invoke(() =>
131+
{
121132
if (profile?.Environment != null)
122133
{
123134
_environmentUrl = profile.Environment.Url;
@@ -128,11 +139,7 @@ private async Task LoadProfileInfoAsync()
128139
{
129140
_statusLabel.Text = "No environment selected. Select a profile with an environment first.";
130141
}
131-
}
132-
catch (Exception ex)
133-
{
134-
_statusLabel.Text = $"Error loading profile: {ex.Message}";
135-
}
142+
});
136143
}
137144

138145
private void SetupKeyboardShortcuts()
@@ -210,17 +217,25 @@ private async Task ExecuteQueryAsync()
210217

211218
var result = await service.ExecuteAsync(request, CancellationToken.None);
212219

213-
_resultsTable.LoadResults(result.Result);
214-
_lastSql = sql;
215-
_lastPagingCookie = result.Result.PagingCookie;
216-
_lastPageNumber = result.Result.PageNumber;
217-
218-
var moreText = result.Result.MoreRecords ? " (more available)" : "";
219-
_statusLabel.Text = $"Returned {result.Result.Count} rows in {result.Result.ExecutionTimeMs}ms{moreText}";
220+
// Update UI on main thread
221+
Application.MainLoop?.Invoke(() =>
222+
{
223+
_resultsTable.LoadResults(result.Result);
224+
_lastSql = sql;
225+
_lastPagingCookie = result.Result.PagingCookie;
226+
_lastPageNumber = result.Result.PageNumber;
227+
228+
var moreText = result.Result.MoreRecords ? " (more available)" : "";
229+
_statusLabel.Text = $"Returned {result.Result.Count} rows in {result.Result.ExecutionTimeMs}ms{moreText}";
230+
});
231+
}
232+
catch (InvalidOperationException ex)
233+
{
234+
Application.MainLoop?.Invoke(() => _statusLabel.Text = $"Error: {ex.Message}");
220235
}
221-
catch (Exception ex)
236+
catch (HttpRequestException ex)
222237
{
223-
_statusLabel.Text = $"Error: {ex.Message}";
238+
Application.MainLoop?.Invoke(() => _statusLabel.Text = $"Network error: {ex.Message}");
224239
}
225240
}
226241

@@ -242,15 +257,23 @@ private async Task OnLoadMoreRequested()
242257

243258
var result = await service.ExecuteAsync(request, CancellationToken.None);
244259

245-
_resultsTable.AddPage(result.Result);
246-
_lastPagingCookie = result.Result.PagingCookie;
247-
_lastPageNumber = result.Result.PageNumber;
260+
// Update UI on main thread
261+
Application.MainLoop?.Invoke(() =>
262+
{
263+
_resultsTable.AddPage(result.Result);
264+
_lastPagingCookie = result.Result.PagingCookie;
265+
_lastPageNumber = result.Result.PageNumber;
248266

249-
_statusLabel.Text = $"Loaded page {_lastPageNumber}";
267+
_statusLabel.Text = $"Loaded page {_lastPageNumber}";
268+
});
269+
}
270+
catch (InvalidOperationException ex)
271+
{
272+
Application.MainLoop?.Invoke(() => _statusLabel.Text = $"Error loading more: {ex.Message}");
250273
}
251-
catch (Exception ex)
274+
catch (HttpRequestException ex)
252275
{
253-
_statusLabel.Text = $"Error loading more: {ex.Message}";
276+
Application.MainLoop?.Invoke(() => _statusLabel.Text = $"Network error: {ex.Message}");
254277
}
255278
}
256279

‎src/PPDS.Cli/Tui/Views/QueryResultsTableView.cs‎

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System.Data;
2+
using System.Net.Http;
23
using PPDS.Cli.Infrastructure;
34
using PPDS.Dataverse.Query;
45
using Terminal.Gui;
@@ -16,6 +17,7 @@ internal sealed class QueryResultsTableView : FrameView
1617
private DataTable _dataTable;
1718
private QueryResult? _lastResult;
1819
private string? _environmentUrl;
20+
private bool _isLoadingMore;
1921

2022
/// <summary>
2123
/// Raised when the user scrolls to the end and more records are available.
@@ -197,19 +199,29 @@ private bool IsNearEnd()
197199

198200
private async Task LoadMoreAsync()
199201
{
200-
if (LoadMoreRequested != null)
201-
{
202-
_statusLabel.Text = $"Loading page {CurrentPageNumber + 1}...";
203-
Application.Refresh();
202+
// Guard against concurrent load requests
203+
if (_isLoadingMore || LoadMoreRequested == null)
204+
return;
204205

205-
try
206-
{
207-
await LoadMoreRequested.Invoke();
208-
}
209-
catch (Exception ex)
210-
{
211-
_statusLabel.Text = $"Error loading: {ex.Message}";
212-
}
206+
_isLoadingMore = true;
207+
_statusLabel.Text = $"Loading page {CurrentPageNumber + 1}...";
208+
Application.Refresh();
209+
210+
try
211+
{
212+
await LoadMoreRequested.Invoke();
213+
}
214+
catch (InvalidOperationException ex)
215+
{
216+
_statusLabel.Text = $"Error loading: {ex.Message}";
217+
}
218+
catch (HttpRequestException ex)
219+
{
220+
_statusLabel.Text = $"Network error: {ex.Message}";
221+
}
222+
finally
223+
{
224+
_isLoadingMore = false;
213225
}
214226
}
215227

0 commit comments

Comments
 (0)