Apply second code review fixes: robustness and correctness

- 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 <noreply@anthropic.com>
This commit is contained in:
Peter Foster
2026-04-13 18:14:00 +01:00
parent 551bed6814
commit 426089fb3e
4 changed files with 122 additions and 70 deletions

View File

@@ -16,7 +16,7 @@ public class AiAssistantService
{ {
private readonly string _apiKey; private readonly string _apiKey;
private readonly string _model; 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"; 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)> public async Task<(string Title, string Description, decimal Price, string PriceReasoning)>
RefineWithCorrectionsAsync(string title, string description, decimal price, string corrections) RefineWithCorrectionsAsync(string title, string description, decimal price, string corrections)
{ {
var priceContext = price > 0
? $"Current price: £{price:F2}\n\n"
: "";
var prompt = var prompt =
"You are an expert eBay UK seller. I have a listing with incorrect details that need fixing.\n\n" + "You are an expert eBay UK seller. I have a listing with incorrect details that need fixing.\n\n" +
$"Current title: {title}\n" + $"Current title: {title}\n" +
$"Current description:\n{description}\n\n" + $"Current description:\n{description}\n\n" +
$"Current price: £{price:F2}\n\n" + priceContext +
$"CORRECTIONS FROM SELLER: {corrections}\n\n" + $"CORRECTIONS FROM SELLER: {corrections}\n\n" +
"Rewrite the listing incorporating these corrections. " + "Rewrite the listing incorporating these corrections. " +
"Return ONLY valid JSON — no markdown, no explanation:\n" + "Return ONLY valid JSON — no markdown, no explanation:\n" +
"{\n" + "{\n" +
" \"title\": \"corrected eBay UK title, max 80 chars, keyword-rich\",\n" + " \"title\": \"corrected eBay UK title, max 80 chars, keyword-rich\",\n" +
" \"description\": \"corrected full plain-text eBay UK description\",\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" + " \"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); var json = await CallAsync(prompt);
@@ -169,10 +174,11 @@ public class AiAssistantService
obj = JObject.Parse(m.Success ? m.Groups[1].Value : json.Trim()); obj = JObject.Parse(m.Success ? m.Groups[1].Value : json.Trim());
} }
var parsedPrice = obj["price_suggested"]?.Value<decimal>() ?? 0;
return ( return (
obj["title"]?.ToString() ?? title, obj["title"]?.ToString() ?? title,
obj["description"]?.ToString() ?? description, obj["description"]?.ToString() ?? description,
obj["price_suggested"]?.Value<decimal>() ?? price, parsedPrice > 0 ? parsedPrice : price, // treat 0 as "not provided", keep original
obj["price_reasoning"]?.ToString() ?? "" obj["price_reasoning"]?.ToString() ?? ""
); );
} }

View File

@@ -18,6 +18,10 @@ public class EbayAuthService
// App-level (client credentials) token — no user login needed, used for Browse API // App-level (client credentials) token — no user login needed, used for Browse API
private string? _appToken; private string? _appToken;
private DateTime _appTokenExpiry = DateTime.MinValue; 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 = private static readonly string[] Scopes =
[ [
@@ -114,19 +118,19 @@ public class EbayAuthService
? "https://api.sandbox.ebay.com/identity/v1/oauth2/token" ? "https://api.sandbox.ebay.com/identity/v1/oauth2/token"
: "https://api.ebay.com/identity/v1/oauth2/token"; : "https://api.ebay.com/identity/v1/oauth2/token";
using var http = new HttpClient();
var credentials = Convert.ToBase64String( var credentials = Convert.ToBase64String(
Encoding.UTF8.GetBytes($"{_settings.ClientId}:{_settings.ClientSecret}")); Encoding.UTF8.GetBytes($"{_settings.ClientId}:{_settings.ClientSecret}"));
http.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", credentials);
var body = new FormUrlEncodedContent(new Dictionary<string, string> using var codeRequest = new HttpRequestMessage(HttpMethod.Post, tokenUrl);
codeRequest.Headers.Authorization = new AuthenticationHeaderValue("Basic", credentials);
codeRequest.Content = new FormUrlEncodedContent(new Dictionary<string, string>
{ {
["grant_type"] = "authorization_code", ["grant_type"] = "authorization_code",
["code"] = code, ["code"] = code,
["redirect_uri"] = _settings.RuName ["redirect_uri"] = _settings.RuName
}); });
var response = await http.PostAsync(tokenUrl, body); var response = await _http.SendAsync(codeRequest);
var json = await response.Content.ReadAsStringAsync(); var json = await response.Content.ReadAsStringAsync();
if (!response.IsSuccessStatusCode) if (!response.IsSuccessStatusCode)
@@ -152,19 +156,19 @@ public class EbayAuthService
? "https://api.sandbox.ebay.com/identity/v1/oauth2/token" ? "https://api.sandbox.ebay.com/identity/v1/oauth2/token"
: "https://api.ebay.com/identity/v1/oauth2/token"; : "https://api.ebay.com/identity/v1/oauth2/token";
using var http = new HttpClient();
var credentials = Convert.ToBase64String( var credentials = Convert.ToBase64String(
Encoding.UTF8.GetBytes($"{_settings.ClientId}:{_settings.ClientSecret}")); Encoding.UTF8.GetBytes($"{_settings.ClientId}:{_settings.ClientSecret}"));
http.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", credentials);
var body = new FormUrlEncodedContent(new Dictionary<string, string> using var refreshRequest = new HttpRequestMessage(HttpMethod.Post, tokenUrl);
refreshRequest.Headers.Authorization = new AuthenticationHeaderValue("Basic", credentials);
refreshRequest.Content = new FormUrlEncodedContent(new Dictionary<string, string>
{ {
["grant_type"] = "refresh_token", ["grant_type"] = "refresh_token",
["refresh_token"] = _token!.RefreshToken, ["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(); var json = await response.Content.ReadAsStringAsync();
if (!response.IsSuccessStatusCode) if (!response.IsSuccessStatusCode)
@@ -180,12 +184,13 @@ public class EbayAuthService
{ {
try try
{ {
using var http = new HttpClient();
http.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", accessToken);
var url = _settings.Sandbox var url = _settings.Sandbox
? "https://apiz.sandbox.ebay.com/commerce/identity/v1/user/" ? "https://apiz.sandbox.ebay.com/commerce/identity/v1/user/"
: "https://apiz.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); var obj = JObject.Parse(json);
return obj["username"]?.ToString() ?? "Unknown"; return obj["username"]?.ToString() ?? "Unknown";
} }
@@ -202,6 +207,14 @@ public class EbayAuthService
/// </summary> /// </summary>
public async Task<string> GetAppTokenAsync() public async Task<string> GetAppTokenAsync()
{ {
// Fast path — no lock needed for a read when token is valid
if (_appToken != null && DateTime.UtcNow < _appTokenExpiry)
return _appToken;
await _appTokenLock.WaitAsync();
try
{
// Re-check inside lock (another thread may have refreshed while we waited)
if (_appToken != null && DateTime.UtcNow < _appTokenExpiry) if (_appToken != null && DateTime.UtcNow < _appTokenExpiry)
return _appToken; return _appToken;
@@ -212,22 +225,22 @@ public class EbayAuthService
? "https://api.sandbox.ebay.com/identity/v1/oauth2/token" ? "https://api.sandbox.ebay.com/identity/v1/oauth2/token"
: "https://api.ebay.com/identity/v1/oauth2/token"; : "https://api.ebay.com/identity/v1/oauth2/token";
using var http = new HttpClient();
var credentials = Convert.ToBase64String( var credentials = Convert.ToBase64String(
Encoding.UTF8.GetBytes($"{_settings.ClientId}:{_settings.ClientSecret}")); Encoding.UTF8.GetBytes($"{_settings.ClientId}:{_settings.ClientSecret}"));
http.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", credentials);
var body = new FormUrlEncodedContent(new Dictionary<string, string> using var request = new HttpRequestMessage(HttpMethod.Post, tokenUrl);
request.Headers.Authorization = new AuthenticationHeaderValue("Basic", credentials);
request.Content = new FormUrlEncodedContent(new Dictionary<string, string>
{ {
["grant_type"] = "client_credentials", ["grant_type"] = "client_credentials",
["scope"] = "https://api.ebay.com/oauth/api_scope" ["scope"] = "https://api.ebay.com/oauth/api_scope"
}); });
var response = await http.PostAsync(tokenUrl, body); var response = await _http.SendAsync(request);
var json = await response.Content.ReadAsStringAsync(); var json = await response.Content.ReadAsStringAsync();
if (!response.IsSuccessStatusCode) if (!response.IsSuccessStatusCode)
throw new HttpRequestException($"App token request failed: {json}"); throw new HttpRequestException($"App token request failed ({(int)response.StatusCode})");
var obj = JObject.Parse(json); var obj = JObject.Parse(json);
_appToken = obj["access_token"]!.ToString(); _appToken = obj["access_token"]!.ToString();
@@ -235,6 +248,11 @@ public class EbayAuthService
_appTokenExpiry = DateTime.UtcNow.AddSeconds(expiresIn - 300); // 5-min buffer _appTokenExpiry = DateTime.UtcNow.AddSeconds(expiresIn - 300); // 5-min buffer
return _appToken; return _appToken;
} }
finally
{
_appTokenLock.Release();
}
}
public void Disconnect() public void Disconnect()
{ {

View File

@@ -10,7 +10,12 @@ public class LivePriceResult
public decimal Min => Prices.Count > 0 ? Prices.Min() : 0; public decimal Min => Prices.Count > 0 ? Prices.Min() : 0;
public decimal Max => Prices.Count > 0 ? Prices.Max() : 0; public decimal Max => Prices.Count > 0 ? Prices.Max() : 0;
public decimal Median => Prices.Count > 0 ? Percentile(50) : 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
// 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) private decimal Percentile(int pct)
{ {
@@ -27,14 +32,16 @@ public class LivePriceResult
public class EbayPriceResearchService public class EbayPriceResearchService
{ {
private readonly EbayAuthService _auth; 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) public EbayPriceResearchService(EbayAuthService auth)
{ {
_auth = auth; _auth = auth;
} }
public async Task<LivePriceResult> GetLivePricesAsync(string query, int limit = 20) public async Task<LivePriceResult> GetLivePricesAsync(string query, int limit = 30)
{ {
if (string.IsNullOrWhiteSpace(query)) if (string.IsNullOrWhiteSpace(query))
return new LivePriceResult(); return new LivePriceResult();
@@ -42,12 +49,13 @@ public class EbayPriceResearchService
var token = await _auth.GetAppTokenAsync(); var token = await _auth.GetAppTokenAsync();
var baseUrl = _auth.BaseUrl; 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, using var request = new HttpRequestMessage(HttpMethod.Get,
$"{baseUrl}/buy/browse/v1/item_summary/search" + $"{baseUrl}/buy/browse/v1/item_summary/search" +
$"?q={Uri.EscapeDataString(query)}" + $"?q={Uri.EscapeDataString(query)}" +
$"&filter=buyingOptions%3A%7BFIXED_PRICE%7D" + $"&filter=buyingOptions%3A%7BFIXED_PRICE%7D" +
$"&limit={limit}" + $"&limit={limit}");
$"&sort=price");
request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token); request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token);
request.Headers.Add("X-EBAY-C-MARKETPLACE-ID", "EBAY_GB"); request.Headers.Add("X-EBAY-C-MARKETPLACE-ID", "EBAY_GB");
@@ -56,7 +64,7 @@ public class EbayPriceResearchService
var json = await response.Content.ReadAsStringAsync(); var json = await response.Content.ReadAsStringAsync();
if (!response.IsSuccessStatusCode) 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 obj = JObject.Parse(json);
var items = obj["itemSummaries"] as JArray ?? new JArray(); 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 }; return new LivePriceResult { Prices = prices };
} }
} }

View File

@@ -248,7 +248,7 @@ public partial class PhotoAnalysisView : UserControl
TitleBox.Text = newTitle; TitleBox.Text = newTitle;
DescriptionBox.Text = newDesc; DescriptionBox.Text = newDesc;
PriceOverride.Value = (double)newPrice; PriceOverride.Value = (double)Math.Round(newPrice, 2); // Issue 6
PriceSuggestedText.Text = newPrice > 0 ? $"£{newPrice:F2}" : "—"; PriceSuggestedText.Text = newPrice > 0 ? $"£{newPrice:F2}" : "—";
_lastResult.Title = newTitle; _lastResult.Title = newTitle;
@@ -338,7 +338,7 @@ public partial class PhotoAnalysisView : UserControl
PriceRangeBar.Visibility = Visibility.Collapsed; PriceRangeBar.Visibility = Visibility.Collapsed;
} }
PriceOverride.Value = (double)r.PriceSuggested; PriceOverride.Value = (double)Math.Round(r.PriceSuggested, 2); // Issue 6
// Price reasoning // Price reasoning
PriceReasoningText.Text = r.PriceReasoning; PriceReasoningText.Text = r.PriceReasoning;
@@ -361,13 +361,20 @@ public partial class PhotoAnalysisView : UserControl
{ {
if (_priceService == null) return; if (_priceService == null) return;
// Show spinner // 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; LivePriceRow.Visibility = Visibility.Visible;
LivePriceSpinner.Visibility = Visibility.Visible; LivePriceSpinner.Visibility = Visibility.Visible;
LivePriceStatus.Text = "Checking live eBay UK prices…"; LivePriceStatus.Text = "Checking live eBay UK prices…";
try
{
var live = await _priceService.GetLivePricesAsync(query); var live = await _priceService.GetLivePricesAsync(query);
if (live.Count == 0) if (live.Count == 0)
@@ -385,7 +392,7 @@ public partial class PhotoAnalysisView : UserControl
// Update suggested price to 40th percentile (competitive but not cheapest) // Update suggested price to 40th percentile (competitive but not cheapest)
var suggested = live.Suggested; var suggested = live.Suggested;
PriceSuggestedText.Text = $"£{suggested:F2}"; 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; if (_lastResult != null) _lastResult.PriceSuggested = suggested;
// Update status label // Update status label
@@ -395,10 +402,14 @@ public partial class PhotoAnalysisView : UserControl
$"(range £{live.Min:F2} £{live.Max:F2})"; $"(range £{live.Min:F2} £{live.Max:F2})";
} }
catch (Exception ex) catch (Exception ex)
{
try
{ {
LivePriceSpinner.Visibility = Visibility.Collapsed; LivePriceSpinner.Visibility = Visibility.Collapsed;
LivePriceStatus.Text = $"Live price lookup unavailable: {ex.Message}"; LivePriceStatus.Text = $"Live price lookup unavailable: {ex.Message}";
} }
catch { /* control may be unloaded by the time catch runs */ }
}
} }
private void TitleBox_TextChanged(object sender, TextChangedEventArgs e) private void TitleBox_TextChanged(object sender, TextChangedEventArgs e)
@@ -602,12 +613,13 @@ public partial class PhotoAnalysisView : UserControl
// ---- Save toast ---- // ---- Save toast ----
private bool _toastAnimating = false;
private void ShowSaveToast() private void ShowSaveToast()
{ {
if (_toastAnimating) return; // Issue 8: always restart — stop any in-progress hold timer and cancel the running
_toastAnimating = true; // 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; SaveToast.Visibility = Visibility.Visible;
@@ -631,7 +643,6 @@ public partial class PhotoAnalysisView : UserControl
slideOut.Completed += (_, _) => slideOut.Completed += (_, _) =>
{ {
SaveToast.Visibility = Visibility.Collapsed; SaveToast.Visibility = Visibility.Collapsed;
_toastAnimating = false;
}; };
ToastTranslate.BeginAnimation(TranslateTransform.YProperty, slideOut); ToastTranslate.BeginAnimation(TranslateTransform.YProperty, slideOut);
}; };