From 426089fb3ed18ed821ca0b36a633359a3eea6179 Mon Sep 17 00:00:00 2001 From: Peter Foster Date: Mon, 13 Apr 2026 18:14:00 +0100 Subject: [PATCH] Apply second code review fixes: robustness and correctness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - AiAssistantService: 90s HTTP timeout; RefineWithCorrections uses 29.99 example price and explicit "Never return 0" instruction; treat returned 0 as missing and keep original price instead - EbayAuthService: shared static HttpClient for all auth/token calls; SemaphoreSlim double-checked locking on GetAppTokenAsync to prevent concurrent token fetches - EbayPriceResearchService: 15s timeout; require ≥5 samples before suggesting; trim top/bottom 10% outliers; BEST_MATCH sort avoids spam/broken listing bias - PhotoAnalysisView: spinner-show inside try (Issue 1); decimal→double casts use Math.Round to avoid 19.99→19.989... drift (Issue 6); Dispatcher.CheckAccess guard for off-thread callers (Issue 7); save toast always restarts instead of silently dropping rapid saves, eliminating any stuck-flag path (Issue 8) Co-Authored-By: Claude Sonnet 4.6 --- .../Services/AiAssistantService.cs | 20 ++-- EbayListingTool/Services/EbayAuthService.cs | 94 +++++++++++-------- .../Services/EbayPriceResearchService.cs | 39 +++++--- .../Views/PhotoAnalysisView.xaml.cs | 39 +++++--- 4 files changed, 122 insertions(+), 70 deletions(-) diff --git a/EbayListingTool/Services/AiAssistantService.cs b/EbayListingTool/Services/AiAssistantService.cs index f697734..86e4cb0 100644 --- a/EbayListingTool/Services/AiAssistantService.cs +++ b/EbayListingTool/Services/AiAssistantService.cs @@ -16,7 +16,7 @@ public class AiAssistantService { private readonly string _apiKey; private readonly string _model; - private static readonly HttpClient _http = new(); + private static readonly HttpClient _http = new() { Timeout = TimeSpan.FromSeconds(90) }; private const string ApiUrl = "https://openrouter.ai/api/v1/chat/completions"; @@ -142,20 +142,25 @@ public class AiAssistantService public async Task<(string Title, string Description, decimal Price, string PriceReasoning)> RefineWithCorrectionsAsync(string title, string description, decimal price, string corrections) { + var priceContext = price > 0 + ? $"Current price: £{price:F2}\n\n" + : ""; + var prompt = "You are an expert eBay UK seller. I have a listing with incorrect details that need fixing.\n\n" + $"Current title: {title}\n" + $"Current description:\n{description}\n\n" + - $"Current price: £{price:F2}\n\n" + + priceContext + $"CORRECTIONS FROM SELLER: {corrections}\n\n" + "Rewrite the listing incorporating these corrections. " + "Return ONLY valid JSON — no markdown, no explanation:\n" + "{\n" + " \"title\": \"corrected eBay UK title, max 80 chars, keyword-rich\",\n" + " \"description\": \"corrected full plain-text eBay UK description\",\n" + - " \"price_suggested\": 0.00,\n" + + " \"price_suggested\": 29.99,\n" + " \"price_reasoning\": \"one sentence why this price\"\n" + - "}"; + "}\n\n" + + "price_suggested MUST be a realistic GBP amount greater than 0. Never return 0."; var json = await CallAsync(prompt); @@ -169,10 +174,11 @@ public class AiAssistantService obj = JObject.Parse(m.Success ? m.Groups[1].Value : json.Trim()); } + var parsedPrice = obj["price_suggested"]?.Value() ?? 0; return ( - obj["title"]?.ToString() ?? title, - obj["description"]?.ToString() ?? description, - obj["price_suggested"]?.Value() ?? price, + obj["title"]?.ToString() ?? title, + obj["description"]?.ToString() ?? description, + parsedPrice > 0 ? parsedPrice : price, // treat 0 as "not provided", keep original obj["price_reasoning"]?.ToString() ?? "" ); } diff --git a/EbayListingTool/Services/EbayAuthService.cs b/EbayListingTool/Services/EbayAuthService.cs index 68a9e6c..6552324 100644 --- a/EbayListingTool/Services/EbayAuthService.cs +++ b/EbayListingTool/Services/EbayAuthService.cs @@ -18,6 +18,10 @@ public class EbayAuthService // App-level (client credentials) token — no user login needed, used for Browse API private string? _appToken; private DateTime _appTokenExpiry = DateTime.MinValue; + private readonly SemaphoreSlim _appTokenLock = new(1, 1); // prevents concurrent token fetches + + // Shared HttpClient for all auth/token calls — avoids socket exhaustion from per-call `new HttpClient()` + private static readonly HttpClient _http = new(); private static readonly string[] Scopes = [ @@ -114,19 +118,19 @@ public class EbayAuthService ? "https://api.sandbox.ebay.com/identity/v1/oauth2/token" : "https://api.ebay.com/identity/v1/oauth2/token"; - using var http = new HttpClient(); var credentials = Convert.ToBase64String( Encoding.UTF8.GetBytes($"{_settings.ClientId}:{_settings.ClientSecret}")); - http.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", credentials); - var body = new FormUrlEncodedContent(new Dictionary + using var codeRequest = new HttpRequestMessage(HttpMethod.Post, tokenUrl); + codeRequest.Headers.Authorization = new AuthenticationHeaderValue("Basic", credentials); + codeRequest.Content = new FormUrlEncodedContent(new Dictionary { ["grant_type"] = "authorization_code", ["code"] = code, ["redirect_uri"] = _settings.RuName }); - var response = await http.PostAsync(tokenUrl, body); + var response = await _http.SendAsync(codeRequest); var json = await response.Content.ReadAsStringAsync(); if (!response.IsSuccessStatusCode) @@ -152,19 +156,19 @@ public class EbayAuthService ? "https://api.sandbox.ebay.com/identity/v1/oauth2/token" : "https://api.ebay.com/identity/v1/oauth2/token"; - using var http = new HttpClient(); var credentials = Convert.ToBase64String( Encoding.UTF8.GetBytes($"{_settings.ClientId}:{_settings.ClientSecret}")); - http.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", credentials); - var body = new FormUrlEncodedContent(new Dictionary + using var refreshRequest = new HttpRequestMessage(HttpMethod.Post, tokenUrl); + refreshRequest.Headers.Authorization = new AuthenticationHeaderValue("Basic", credentials); + refreshRequest.Content = new FormUrlEncodedContent(new Dictionary { - ["grant_type"] = "refresh_token", + ["grant_type"] = "refresh_token", ["refresh_token"] = _token!.RefreshToken, - ["scope"] = string.Join(" ", Scopes) + ["scope"] = string.Join(" ", Scopes) }); - var response = await http.PostAsync(tokenUrl, body); + var response = await _http.SendAsync(refreshRequest); var json = await response.Content.ReadAsStringAsync(); if (!response.IsSuccessStatusCode) @@ -180,12 +184,13 @@ public class EbayAuthService { try { - using var http = new HttpClient(); - http.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", accessToken); var url = _settings.Sandbox ? "https://apiz.sandbox.ebay.com/commerce/identity/v1/user/" : "https://apiz.ebay.com/commerce/identity/v1/user/"; - var json = await http.GetStringAsync(url); + using var idRequest = new HttpRequestMessage(HttpMethod.Get, url); + idRequest.Headers.Authorization = new AuthenticationHeaderValue("Bearer", accessToken); + var idResponse = await _http.SendAsync(idRequest); + var json = await idResponse.Content.ReadAsStringAsync(); var obj = JObject.Parse(json); return obj["username"]?.ToString() ?? "Unknown"; } @@ -202,38 +207,51 @@ public class EbayAuthService /// public async Task GetAppTokenAsync() { + // Fast path — no lock needed for a read when token is valid if (_appToken != null && DateTime.UtcNow < _appTokenExpiry) return _appToken; - if (string.IsNullOrEmpty(_settings.ClientId) || string.IsNullOrEmpty(_settings.ClientSecret)) - throw new InvalidOperationException("eBay Client ID / Secret not configured in appsettings.json."); - - var tokenUrl = _settings.Sandbox - ? "https://api.sandbox.ebay.com/identity/v1/oauth2/token" - : "https://api.ebay.com/identity/v1/oauth2/token"; - - using var http = new HttpClient(); - var credentials = Convert.ToBase64String( - Encoding.UTF8.GetBytes($"{_settings.ClientId}:{_settings.ClientSecret}")); - http.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", credentials); - - var body = new FormUrlEncodedContent(new Dictionary + await _appTokenLock.WaitAsync(); + try { - ["grant_type"] = "client_credentials", - ["scope"] = "https://api.ebay.com/oauth/api_scope" - }); + // Re-check inside lock (another thread may have refreshed while we waited) + if (_appToken != null && DateTime.UtcNow < _appTokenExpiry) + return _appToken; - var response = await http.PostAsync(tokenUrl, body); - var json = await response.Content.ReadAsStringAsync(); + if (string.IsNullOrEmpty(_settings.ClientId) || string.IsNullOrEmpty(_settings.ClientSecret)) + throw new InvalidOperationException("eBay Client ID / Secret not configured in appsettings.json."); - if (!response.IsSuccessStatusCode) - throw new HttpRequestException($"App token request failed: {json}"); + var tokenUrl = _settings.Sandbox + ? "https://api.sandbox.ebay.com/identity/v1/oauth2/token" + : "https://api.ebay.com/identity/v1/oauth2/token"; - var obj = JObject.Parse(json); - _appToken = obj["access_token"]!.ToString(); - var expiresIn = obj["expires_in"]?.Value() ?? 7200; - _appTokenExpiry = DateTime.UtcNow.AddSeconds(expiresIn - 300); // 5-min buffer - return _appToken; + var credentials = Convert.ToBase64String( + Encoding.UTF8.GetBytes($"{_settings.ClientId}:{_settings.ClientSecret}")); + + using var request = new HttpRequestMessage(HttpMethod.Post, tokenUrl); + request.Headers.Authorization = new AuthenticationHeaderValue("Basic", credentials); + request.Content = new FormUrlEncodedContent(new Dictionary + { + ["grant_type"] = "client_credentials", + ["scope"] = "https://api.ebay.com/oauth/api_scope" + }); + + var response = await _http.SendAsync(request); + var json = await response.Content.ReadAsStringAsync(); + + if (!response.IsSuccessStatusCode) + throw new HttpRequestException($"App token request failed ({(int)response.StatusCode})"); + + var obj = JObject.Parse(json); + _appToken = obj["access_token"]!.ToString(); + var expiresIn = obj["expires_in"]?.Value() ?? 7200; + _appTokenExpiry = DateTime.UtcNow.AddSeconds(expiresIn - 300); // 5-min buffer + return _appToken; + } + finally + { + _appTokenLock.Release(); + } } public void Disconnect() diff --git a/EbayListingTool/Services/EbayPriceResearchService.cs b/EbayListingTool/Services/EbayPriceResearchService.cs index f45095f..9fa47d2 100644 --- a/EbayListingTool/Services/EbayPriceResearchService.cs +++ b/EbayListingTool/Services/EbayPriceResearchService.cs @@ -5,12 +5,17 @@ namespace EbayListingTool.Services; public class LivePriceResult { - public List Prices { get; init; } = new(); - public int Count => Prices.Count; - public decimal Min => Prices.Count > 0 ? Prices.Min() : 0; - public decimal Max => Prices.Count > 0 ? Prices.Max() : 0; - public decimal Median => Prices.Count > 0 ? Percentile(50) : 0; - public decimal Suggested => Prices.Count > 0 ? Percentile(40) : 0; // 40th pct: competitive but not lowest + public List Prices { get; init; } = new(); + public int Count => Prices.Count; + public decimal Min => Prices.Count > 0 ? Prices.Min() : 0; + public decimal Max => Prices.Count > 0 ? Prices.Max() : 0; + public decimal Median => Prices.Count > 0 ? Percentile(50) : 0; + + // Require ≥5 samples before offering a suggestion; 40th pct = competitive but not cheapest + public decimal Suggested => Prices.Count >= MinSampleSize ? Percentile(40) : 0; + public bool HasSuggestion => Prices.Count >= MinSampleSize; + + internal const int MinSampleSize = 5; private decimal Percentile(int pct) { @@ -27,14 +32,16 @@ public class LivePriceResult public class EbayPriceResearchService { private readonly EbayAuthService _auth; - private static readonly HttpClient _http = new(); + + // 15-second timeout — price lookup is non-critical; don't leave spinner hanging + private static readonly HttpClient _http = new() { Timeout = TimeSpan.FromSeconds(15) }; public EbayPriceResearchService(EbayAuthService auth) { _auth = auth; } - public async Task GetLivePricesAsync(string query, int limit = 20) + public async Task GetLivePricesAsync(string query, int limit = 30) { if (string.IsNullOrWhiteSpace(query)) return new LivePriceResult(); @@ -42,12 +49,13 @@ public class EbayPriceResearchService var token = await _auth.GetAppTokenAsync(); var baseUrl = _auth.BaseUrl; + // Omit sort= to use eBay BEST_MATCH — price-sorted results are biased toward + // the cheapest (broken/spam) listings; BEST_MATCH gives a more representative sample using var request = new HttpRequestMessage(HttpMethod.Get, $"{baseUrl}/buy/browse/v1/item_summary/search" + $"?q={Uri.EscapeDataString(query)}" + $"&filter=buyingOptions%3A%7BFIXED_PRICE%7D" + - $"&limit={limit}" + - $"&sort=price"); + $"&limit={limit}"); request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token); request.Headers.Add("X-EBAY-C-MARKETPLACE-ID", "EBAY_GB"); @@ -56,7 +64,7 @@ public class EbayPriceResearchService var json = await response.Content.ReadAsStringAsync(); if (!response.IsSuccessStatusCode) - throw new HttpRequestException($"eBay Browse API error ({(int)response.StatusCode}): {json}"); + throw new HttpRequestException($"eBay Browse API error ({(int)response.StatusCode})"); var obj = JObject.Parse(json); var items = obj["itemSummaries"] as JArray ?? new JArray(); @@ -75,6 +83,15 @@ public class EbayPriceResearchService } } + // Trim top and bottom 10% to remove outliers (spammers, broken items, typos). + // Only trim when there are enough data points to make it worthwhile. + if (prices.Count >= 10) + { + prices.Sort(); + int trim = (int)Math.Floor(prices.Count * 0.10); + prices = prices.Skip(trim).Take(prices.Count - 2 * trim).ToList(); + } + return new LivePriceResult { Prices = prices }; } } diff --git a/EbayListingTool/Views/PhotoAnalysisView.xaml.cs b/EbayListingTool/Views/PhotoAnalysisView.xaml.cs index a679690..224c041 100644 --- a/EbayListingTool/Views/PhotoAnalysisView.xaml.cs +++ b/EbayListingTool/Views/PhotoAnalysisView.xaml.cs @@ -248,7 +248,7 @@ public partial class PhotoAnalysisView : UserControl TitleBox.Text = newTitle; DescriptionBox.Text = newDesc; - PriceOverride.Value = (double)newPrice; + PriceOverride.Value = (double)Math.Round(newPrice, 2); // Issue 6 PriceSuggestedText.Text = newPrice > 0 ? $"£{newPrice:F2}" : "—"; _lastResult.Title = newTitle; @@ -338,7 +338,7 @@ public partial class PhotoAnalysisView : UserControl PriceRangeBar.Visibility = Visibility.Collapsed; } - PriceOverride.Value = (double)r.PriceSuggested; + PriceOverride.Value = (double)Math.Round(r.PriceSuggested, 2); // Issue 6 // Price reasoning PriceReasoningText.Text = r.PriceReasoning; @@ -361,13 +361,20 @@ public partial class PhotoAnalysisView : UserControl { if (_priceService == null) return; - // Show spinner - LivePriceRow.Visibility = Visibility.Visible; - LivePriceSpinner.Visibility = Visibility.Visible; - LivePriceStatus.Text = "Checking live eBay UK prices…"; + // Issue 7: guard against off-thread callers (fire-and-forget may lose sync context) + if (!Dispatcher.CheckAccess()) + { + await Dispatcher.InvokeAsync(() => UpdateLivePricesAsync(query)).Task.Unwrap(); + return; + } try { + // Issue 1: spinner-show inside try so a disposed control doesn't crash the caller + LivePriceRow.Visibility = Visibility.Visible; + LivePriceSpinner.Visibility = Visibility.Visible; + LivePriceStatus.Text = "Checking live eBay UK prices…"; + var live = await _priceService.GetLivePricesAsync(query); if (live.Count == 0) @@ -385,7 +392,7 @@ public partial class PhotoAnalysisView : UserControl // Update suggested price to 40th percentile (competitive but not cheapest) var suggested = live.Suggested; PriceSuggestedText.Text = $"£{suggested:F2}"; - PriceOverride.Value = (double)suggested; + PriceOverride.Value = (double)Math.Round(suggested, 2); // Issue 6: avoid decimal→double drift if (_lastResult != null) _lastResult.PriceSuggested = suggested; // Update status label @@ -396,8 +403,12 @@ public partial class PhotoAnalysisView : UserControl } catch (Exception ex) { - LivePriceSpinner.Visibility = Visibility.Collapsed; - LivePriceStatus.Text = $"Live price lookup unavailable: {ex.Message}"; + try + { + LivePriceSpinner.Visibility = Visibility.Collapsed; + LivePriceStatus.Text = $"Live price lookup unavailable: {ex.Message}"; + } + catch { /* control may be unloaded by the time catch runs */ } } } @@ -602,12 +613,13 @@ public partial class PhotoAnalysisView : UserControl // ---- Save toast ---- - private bool _toastAnimating = false; - private void ShowSaveToast() { - if (_toastAnimating) return; - _toastAnimating = true; + // Issue 8: always restart — stop any in-progress hold timer and cancel the running + // animation so the flag can never get permanently stuck and rapid saves feel responsive. + _holdTimer?.Stop(); + _holdTimer = null; + ToastTranslate.BeginAnimation(TranslateTransform.YProperty, null); // cancel current animation SaveToast.Visibility = Visibility.Visible; @@ -631,7 +643,6 @@ public partial class PhotoAnalysisView : UserControl slideOut.Completed += (_, _) => { SaveToast.Visibility = Visibility.Collapsed; - _toastAnimating = false; }; ToastTranslate.BeginAnimation(TranslateTransform.YProperty, slideOut); };