Optimize startup and background page updates - #11
Conversation
|
Warning Review limit reached
Next review available in: 32 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe release process now packages the main application with its web assets. The application uses shared WebView2 state and installation-relative paths. MainForm defers startup services and coordinates configurable, cached, revisioned trade and login data with the dashboard. ChangesApplication publishing and WebView runtime
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant MainForm
participant WebViewEnvironmentProvider
participant SteamServices
Dashboard->>MainForm: Send tab or cache/refresh command
MainForm->>WebViewEnvironmentProvider: GetAsync()
WebViewEnvironmentProvider-->>MainForm: Return shared WebView2 environment
MainForm->>SteamServices: Fetch trade or login data
SteamServices-->>MainForm: Return account and request results
MainForm->>Dashboard: Publish cached data with revision
Dashboard->>Dashboard: Render accepted response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
Steam Desktop Authenticator/InputForm.cs (1)
61-61: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnhandled faults from the shared WebView2 environment in both dialog forms. Both forms call
WebViewEnvironmentProvider.GetAsync()from anasync void SetupModernUIwith no error handling. The provider caches one task, so a single failure (missing WebView2 runtime, locked user-data folder) faults every form the same way and leaves each dialog on its loading panel.
Steam Desktop Authenticator/InputForm.cs#L61-L61: wrap theEnsureCoreWebView2Asynccall in a try/catch, log throughDiagnosticErrorLogger, setlblLoading.Textto an explicit error, and return.Steam Desktop Authenticator/LoginForm.cs#L78-L78: apply the same try/catch and error message around theEnsureCoreWebView2Asynccall.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Steam` Desktop Authenticator/InputForm.cs at line 61, Handle failures from WebViewEnvironmentProvider.GetAsync within SetupModernUI in both Steam Desktop Authenticator/InputForm.cs at lines 61-61 and Steam Desktop Authenticator/LoginForm.cs at lines 78-78: wrap EnsureCoreWebView2Async in try/catch, log the exception through DiagnosticErrorLogger, set lblLoading.Text to an explicit error message, and return so each dialog stops loading cleanly.Steam Desktop Authenticator/MainForm.cs (3)
1212-1234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
IsRecentlyResolvedto reflect that it mutates the dictionary.The name states a query, but the method removes expired entries from
resolvedRequests. A future caller can use it inside an active enumeration of the same dictionary and get anInvalidOperationException. ConsiderPruneAndCheckRecentlyResolved, or split the pruning into a separatePruneRecentlyResolvedcall.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Steam` Desktop Authenticator/MainForm.cs around lines 1212 - 1234, Rename IsRecentlyResolved to a mutation-reflecting name such as PruneAndCheckRecentlyResolved, and update every caller accordingly. Preserve its existing expiration cleanup and key-presence check; do not use the query-style name for this dictionary-mutating method.
1507-1515: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueRemove the unused
LoadLoginActionsAsyncmethod.The method has no callers. Its missing
revisiondoes not affect the current runtime path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Steam` Desktop Authenticator/MainForm.cs around lines 1507 - 1515, Remove the unused LoadLoginActionsAsync method from MainForm, leaving the active PublishCachedLoginActionsAsync path and its revision handling unchanged.
1325-1339: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPass the confirmations array directly to
loadConfirmations.The single-quoted wrapper corrupts escaped quotes, backslashes, and newlines before
JSON.parsereceives the payload.StringEscapeHandling.EscapeHtmldoes not prevent this, andReplace("'", "\\'")is redundant.Use the direct-object pattern from
PublishCachedLoginActionsAsync, then updateloadConfirmationsto accept an array.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Steam` Desktop Authenticator/MainForm.cs around lines 1325 - 1339, The confirmation payload in the `loadConfirmations` invocation is incorrectly wrapped as a single-quoted string and manually escaped. Follow the direct-object pattern from `PublishCachedLoginActionsAsync`: pass the serialized confirmations array directly as the JavaScript argument, remove the `jsEscaped` and apostrophe replacement, and update `loadConfirmations` to accept the array without reparsing a string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 63: Update the Visual Studio alternative in the README to instruct users
to use Publish rather than Build for the Steam Desktop Authenticator project,
matching the documented runtime, output, and publish settings so the
publish/ASDA executable is created.
In `@Steam` Desktop Authenticator/ApplicationPaths.cs:
- Around line 10-12: Update the InstallDirectory property to use
Path.TrimEndingDirectorySeparator on AppContext.BaseDirectory instead of
TrimEnd, preserving the directory separator for volume-root paths such as C:\
while still removing redundant trailing separators elsewhere.
In `@Steam` Desktop Authenticator/LoginForm.cs:
- Around line 105-106: Update the login.html navigation handler in
MainForm.SetupModernUI to check args.IsSuccess after navigation, matching the
existing failure handling in MainForm.SetupModernUI, and report navigation
failures while preserving the successful loading flow.
In `@Steam` Desktop Authenticator/MainForm.cs:
- Around line 1303-1312: Guard null sessions in both WebView-driven paths: in
IsTradeCacheCompleteForSelection, filter accounts to those with non-null Session
before checking loadedTradeConfirmationAccounts; in Steam Desktop
Authenticator/MainForm.cs lines 1570-1571, update RespondToLoginActionAsync’s
account validation to reject account?.Session == null before
BuildLoginRequestKey runs.
- Around line 49-52: Update loadAccountsList() to prune all account-scoped
caches after determining the active Steam IDs and login names: remove stale
entries from loadedTradeConfirmationAccounts and loadedTradeConfirmations by
Steam ID, and from unavailableLoginAccounts by account name. Preserve entries
belonging to currently loaded accounts so cached login status and trade
confirmations remain available.
- Around line 2033-2040: Update the navigation-failure branch in MainForm’s
WebView2 initialization flow to invoke StartBackgroundServicesAfterUiReady
before returning. Preserve the existing cleanup, error message, and early return
so timerSteamGuard, loadSettings, and ConfigureLoginActionsMonitor still
initialize when UI navigation fails.
- Around line 2352-2354: Move the confirmationsSemaphore.WaitAsync acquisition
into the try/finally flow following tradeLoadSemaphore acquisition, and ensure
cleanup releases each semaphore only if that semaphore was successfully
acquired. Preserve the existing early return when tradeLoadSemaphore cannot be
acquired, and update the surrounding method to prevent an exception during
confirmation acquisition from leaving tradeLoadSemaphore held.
- Around line 1570-1571: Update the account validation in the
respond_login_action handling flow to reject accounts whose Session is null
before calling BuildLoginRequestKey. Extend the existing guard around account
and pendingLoginRequests so account.Session is validated first, while preserving
the current behavior for missing accounts and requests.
- Around line 2255-2257: Update the refresh_login_actions branch in MainForm to
call a new RefreshLoginActionsAsync helper instead of
MonitorLoginActionsSafelyAsync directly. Implement the helper to await
MonitorLoginActionsSafelyAsync first, then await PublishCachedLoginActionsAsync
unconditionally so cached data is published after early returns and regardless
of web-tab activity.
In `@Steam` Desktop Authenticator/wwwroot/index.html:
- Line 541: Update both refresh action handlers, including the
refresh_login_actions and refresh_trades postMessage call sites, to show the
existing spinner before posting the refresh request. Hide the spinner when
refreshed data arrives, and add a timeout fallback so refresh_login_actions also
stops spinning when the host returns without publishing data.
---
Nitpick comments:
In `@Steam` Desktop Authenticator/InputForm.cs:
- Line 61: Handle failures from WebViewEnvironmentProvider.GetAsync within
SetupModernUI in both Steam Desktop Authenticator/InputForm.cs at lines 61-61
and Steam Desktop Authenticator/LoginForm.cs at lines 78-78: wrap
EnsureCoreWebView2Async in try/catch, log the exception through
DiagnosticErrorLogger, set lblLoading.Text to an explicit error message, and
return so each dialog stops loading cleanly.
In `@Steam` Desktop Authenticator/MainForm.cs:
- Around line 1212-1234: Rename IsRecentlyResolved to a mutation-reflecting name
such as PruneAndCheckRecentlyResolved, and update every caller accordingly.
Preserve its existing expiration cleanup and key-presence check; do not use the
query-style name for this dictionary-mutating method.
- Around line 1507-1515: Remove the unused LoadLoginActionsAsync method from
MainForm, leaving the active PublishCachedLoginActionsAsync path and its
revision handling unchanged.
- Around line 1325-1339: The confirmation payload in the `loadConfirmations`
invocation is incorrectly wrapped as a single-quoted string and manually
escaped. Follow the direct-object pattern from `PublishCachedLoginActionsAsync`:
pass the serialized confirmations array directly as the JavaScript argument,
remove the `jsEscaped` and apostrophe replacement, and update
`loadConfirmations` to accept the array without reparsing a string.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f9292d4-3881-44d3-a03d-3438b0f771b7
⛔ Files ignored due to path filters (3)
github-banner.jpgis excluded by!**/*.jpgscreenshot main page.pngis excluded by!**/*.pngscreenshot settings.pngis excluded by!**/*.png
📒 Files selected for processing (11)
.github/workflows/release.ymlREADME.mdSteam Desktop Authenticator/ApplicationPaths.csSteam Desktop Authenticator/InputForm.csSteam Desktop Authenticator/LoginForm.csSteam Desktop Authenticator/MainForm.csSteam Desktop Authenticator/Manifest.csSteam Desktop Authenticator/Program.csSteam Desktop Authenticator/Steam Desktop Authenticator.csprojSteam Desktop Authenticator/WebViewEnvironmentProvider.csSteam Desktop Authenticator/wwwroot/index.html
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Steam Desktop Authenticator/MainForm.cs (1)
2341-2353: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReload when the selected account changes during an active trade load.
When a user selects account B while account A is loading, the second
LoadTradesAsyncreturns at Line 2353. The first load does not fetch B. The page can then show stale or empty cached data for B until the user refreshes again.Capture the selection for each load. After releasing the semaphore, queue one reload if
tradeAccountSelectionchanged.Proposed fix
private async Task LoadTradesAsync(string selectedAccountName = null) { if (!String.IsNullOrWhiteSpace(selectedAccountName)) tradeAccountSelection = selectedAccountName; + string selectionToLoad = tradeAccountSelection; ... - SteamGuardAccount[] accountsToLoad = tradeAccountSelection == "all" + SteamGuardAccount[] accountsToLoad = selectionToLoad == "all" ? allAccounts - : allAccounts.Where(account => account.AccountName == tradeAccountSelection).ToArray(); + : allAccounts.Where(account => account.AccountName == selectionToLoad).ToArray(); ... finally { confirmationsSemaphore.Release(); tradeLoadSemaphore.Release(); + if (!String.Equals(selectionToLoad, tradeAccountSelection, StringComparison.Ordinal)) + _ = LoadTradesAsync(); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Steam` Desktop Authenticator/MainForm.cs around lines 2341 - 2353, Update LoadTradesAsync to capture the account selection at the start of each load and, when semaphore acquisition fails because another load is active, record that a reload is needed if tradeAccountSelection differs. After the active load releases tradeLoadSemaphore, compare the captured selection with the current selection and queue exactly one LoadTradesAsync reload when it changed, ensuring the latest account is fetched without creating repeated reloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Steam` Desktop Authenticator/InputForm.cs:
- Line 105: Update the input.html navigation completion handler near the
htmlPath setup to check args.IsSuccess before swapping controls. On failure,
keep loadingPanel visible, display the load error, and return without hiding
fallback controls or showing webView; only perform the existing successful
navigation control changes when navigation succeeds.
---
Outside diff comments:
In `@Steam` Desktop Authenticator/MainForm.cs:
- Around line 2341-2353: Update LoadTradesAsync to capture the account selection
at the start of each load and, when semaphore acquisition fails because another
load is active, record that a reload is needed if tradeAccountSelection differs.
After the active load releases tradeLoadSemaphore, compare the captured
selection with the current selection and queue exactly one LoadTradesAsync
reload when it changed, ensuring the latest account is fetched without creating
repeated reloads.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 08d4b29b-42f3-4d51-a1d6-f97fde0bec23
⛔ Files ignored due to path filters (3)
github-banner.jpgis excluded by!**/*.jpgscreenshot main page.pngis excluded by!**/*.pngscreenshot settings.pngis excluded by!**/*.png
📒 Files selected for processing (11)
.github/workflows/release.ymlREADME.mdSteam Desktop Authenticator/ApplicationPaths.csSteam Desktop Authenticator/InputForm.csSteam Desktop Authenticator/LoginForm.csSteam Desktop Authenticator/MainForm.csSteam Desktop Authenticator/Manifest.csSteam Desktop Authenticator/Program.csSteam Desktop Authenticator/Steam Desktop Authenticator.csprojSteam Desktop Authenticator/WebViewEnvironmentProvider.csSteam Desktop Authenticator/wwwroot/index.html
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Steam Desktop Authenticator/MainForm.cs (1)
2323-2335: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not drop a newer trade selection while a load is active.
LoadTradesAsyncchangestradeAccountSelectionbefore it attemptstradeLoadSemaphore. If a load for account A is awaiting Steam and the user refreshes account B, the second call returns at Line 2335. The first call can finish with account A data, publish using the now-global account B selection, and never fetch B.Capture the requested selection in a local variable. Use that variable to build
accountsToLoad. After releasing the semaphore, schedule one reload whentradeAccountSelectionchanged during the load.Proposed fix
private async Task LoadTradesAsync(string selectedAccountName = null) { + string requestedSelection = !String.IsNullOrWhiteSpace(selectedAccountName) + ? selectedAccountName + : tradeAccountSelection; + if (!String.IsNullOrWhiteSpace(selectedAccountName)) tradeAccountSelection = selectedAccountName; ... - SteamGuardAccount[] accountsToLoad = tradeAccountSelection == "all" + SteamGuardAccount[] accountsToLoad = requestedSelection == "all" ? allAccounts - : allAccounts.Where(account => account.AccountName == tradeAccountSelection).ToArray(); + : allAccounts.Where(account => account.AccountName == requestedSelection).ToArray(); ... finally { confirmationsSemaphore.Release(); tradeLoadSemaphore.Release(); + if (!String.Equals(requestedSelection, tradeAccountSelection, StringComparison.Ordinal)) + _ = LoadTradesAsync(tradeAccountSelection); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Steam` Desktop Authenticator/MainForm.cs around lines 2323 - 2335, Update LoadTradesAsync to capture the requested account selection locally before changing shared state, and use that local value when constructing accountsToLoad. If semaphore acquisition fails because another load is active, ensure the completed load schedules one follow-up reload whenever tradeAccountSelection changed during it, so a newer selection is not dropped.
♻️ Duplicate comments (1)
Steam Desktop Authenticator/MainForm.cs (1)
2237-2239: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake explicit login refresh work when monitoring is disabled.
When
LoginActionMonitoringEnabledisfalse,MonitorLoginActionsAsyncreturns at Lines 978-979. Therefresh_login_actionsaction then does not fetch current requests. It also does not publish cached state when rate limiting orloginActionsSemaphorecauses an early return.Add a refresh path that performs fetch-only reconciliation in manual mode, updates
pendingLoginRequestsandunavailableLoginAccounts, and always callsPublishCachedLoginActionsAsyncafter the attempt. Do not run automatic decisions from that fetch-only path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Steam` Desktop Authenticator/MainForm.cs around lines 2237 - 2239, Update the refresh_login_actions branch in MainForm to use a manual fetch-only reconciliation path when LoginActionMonitoringEnabled is false, rather than relying on MonitorLoginActionsAsync. Ensure this path updates pendingLoginRequests and unavailableLoginAccounts, skips automatic decisions, and always invokes PublishCachedLoginActionsAsync after the attempt, including rate-limit or loginActionsSemaphore early returns; preserve normal monitoring behavior when enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Steam` Desktop Authenticator/Manifest.cs:
- Around line 36-40: Update GetManifest() to read legacy periodic_checking and
periodic_checking_interval values before applying defaults, mapping them to
TradeConfirmationCustomIntervalEnabled and TradeConfirmationCheckInterval.
Explicitly handle periodic_checking_checkall as the new monitor always scans all
accounts, then normalize the migrated values so startup Save() preserves them
instead of writing defaults.
---
Outside diff comments:
In `@Steam` Desktop Authenticator/MainForm.cs:
- Around line 2323-2335: Update LoadTradesAsync to capture the requested account
selection locally before changing shared state, and use that local value when
constructing accountsToLoad. If semaphore acquisition fails because another load
is active, ensure the completed load schedules one follow-up reload whenever
tradeAccountSelection changed during it, so a newer selection is not dropped.
---
Duplicate comments:
In `@Steam` Desktop Authenticator/MainForm.cs:
- Around line 2237-2239: Update the refresh_login_actions branch in MainForm to
use a manual fetch-only reconciliation path when LoginActionMonitoringEnabled is
false, rather than relying on MonitorLoginActionsAsync. Ensure this path updates
pendingLoginRequests and unavailableLoginAccounts, skips automatic decisions,
and always invokes PublishCachedLoginActionsAsync after the attempt, including
rate-limit or loginActionsSemaphore early returns; preserve normal monitoring
behavior when enabled.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: e469daac-723b-4a66-bd0e-5477043149a3
📒 Files selected for processing (5)
Steam Desktop Authenticator/MainForm.csSteam Desktop Authenticator/Manifest.csSteam Desktop Authenticator/Program.csSteam Desktop Authenticator/WindowsStartup.csSteam Desktop Authenticator/wwwroot/index.html
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Line 100: Update the release workflow’s run steps that use
github.event.inputs.release_type or github.event.inputs.version, including the
Compress-Archive command, to reference environment variables via
$env:RELEASE_TYPE and $env:VERSION instead. Define those environment variables
from the workflow inputs at the appropriate job or step scope, and remove direct
interpolation of the free-form version input from PowerShell source.
In `@Steam` Desktop Authenticator/MainForm.cs:
- Line 2039: Update the MainForm initialization flow around
EnsureCoreWebView2Async to catch initialization failures and follow the existing
local recovery behavior used by InputForm and LoginForm. Ensure the loading UI
is dismissed and StartBackgroundServicesAfterUiReady still runs or the failure
is otherwise handled consistently, without allowing the async void method to
exit before navigation handlers and background services are initialized.
In `@Steam` Desktop Authenticator/wwwroot/index.html:
- Line 573: Update the trade-cache loading flow around the `postMessage` call
and `loadConfirmations` so each request carries the selected account or a unique
request token, and ensure `loadConfirmations` rejects results that no longer
match the current selection before updating the UI.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d3a6d40-8a2d-49fe-9543-0fcb78d5a19a
⛔ Files ignored due to path filters (3)
github-banner.jpgis excluded by!**/*.jpgscreenshot main page.pngis excluded by!**/*.pngscreenshot settings.pngis excluded by!**/*.png
📒 Files selected for processing (12)
.github/workflows/release.ymlREADME.mdSteam Desktop Authenticator/ApplicationPaths.csSteam Desktop Authenticator/InputForm.csSteam Desktop Authenticator/LoginForm.csSteam Desktop Authenticator/MainForm.csSteam Desktop Authenticator/Manifest.csSteam Desktop Authenticator/Program.csSteam Desktop Authenticator/Steam Desktop Authenticator.csprojSteam Desktop Authenticator/WebViewEnvironmentProvider.csSteam Desktop Authenticator/WindowsStartup.csSteam Desktop Authenticator/wwwroot/index.html
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Steam Desktop Authenticator/MainForm.cs (3)
125-137: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCap the monitor retry delay independently of the poll interval.
FetchTradeConfirmationsForMonitorAsyncpasses the poll interval asretryDelaywithretryCount = 3. The interval is user-configurable up to 3600 seconds. A failing account then keepsconfirmationsSemaphoreheld for up to three interval delays. During that timetimerTradesPopup_Tickreturns immediately at Line 859, andLoadTradesAsyncblocks onawait confirmationsSemaphore.WaitAsync()at Line 2361, so the trades page stops refreshing for a long period.Use a separate bounded backoff for retries.
🐛 Proposed fix
private Task<Confirmation[]> FetchTradeConfirmationsForMonitorAsync(SteamGuardAccount account) { - int seconds = GetTradeConfirmationMonitorIntervalSeconds(); - return FetchTradeConfirmationsAsync(account, TimeSpan.FromSeconds(seconds), 3); + int seconds = Math.Min(GetTradeConfirmationMonitorIntervalSeconds(), 15); + return FetchTradeConfirmationsAsync(account, TimeSpan.FromSeconds(seconds), 3); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Steam` Desktop Authenticator/MainForm.cs around lines 125 - 137, Update FetchTradeConfirmationsForMonitorAsync so the retryDelay is independent of GetTradeConfirmationMonitorIntervalSeconds and uses a short bounded backoff, while preserving the existing retryCount of 3 and the user-configured interval for polling. Keep GetTradeConfirmationMonitorIntervalSeconds responsible only for the monitor poll interval.
907-918: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the cached confirmation set between the monitor and the page loader.
The monitor caches only
pendingConfirmationsat Line 916, so auto-confirm-eligible entries are excluded.LoadTradesAsynccaches every fetched confirmation at Line 2384. A manual refresh therefore lists market or trade confirmations that the monitor auto-accepts. The user can click accept on an entry that Steam already resolved, and the entry disappears on the next monitor tick.Filter the auto-confirm types in
LoadTradesAsync, or cache all confirmations in the monitor, so both paths publish the same set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Steam` Desktop Authenticator/MainForm.cs around lines 907 - 918, The trade confirmation cache is populated inconsistently between the monitor and LoadTradesAsync, allowing auto-confirmed entries to remain visible after resolution. Update the monitor flow around CacheTradeConfirmations to cache the same confirmation set as LoadTradesAsync, or apply the identical auto-confirm filtering in LoadTradesAsync, so both paths publish matching user-visible entries.
2028-2040: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRetry monitoring setup after manifest initialization
If
EnsureCoreWebView2Asyncfails beforeMainForm_Shown, the latch is set whilemanifestis null. Both monitoring setup methods then return, andMainForm_Showndoes not retry them. Trade and login monitoring remain disabled for the session. Start these services after manifest initialization or retry them fromMainForm_Shown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Steam` Desktop Authenticator/MainForm.cs around lines 2028 - 2040, Update the WebView2 initialization failure path around EnsureCoreWebView2Async so monitoring setup is deferred until manifest initialization completes. Ensure StartBackgroundServicesAfterUiReady or MainForm_Shown retries both trade and login monitoring after manifest is available, rather than leaving them disabled for the session.
🧹 Nitpick comments (1)
Steam Desktop Authenticator/MainForm.cs (1)
1624-1625: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
RefreshLoginActionsAsyncafter a manual login action.
MonitorLoginActionsSafelyAsyncreturns at Line 978 whenmanifest.LoginActionMonitoringEnabledis false. With monitoring disabled, the list is never re-fetched after a manual approve or deny, so requests that Steam resolved elsewhere stay inpendingLoginRequestsuntil the user presses refresh.RefreshLoginActionsAsyncselects the manual fetch in that case.♻️ Proposed change
await PublishCachedLoginActionsAsync(); - _ = MonitorLoginActionsSafelyAsync(); + _ = RefreshLoginActionsAsync();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Steam` Desktop Authenticator/MainForm.cs around lines 1624 - 1625, After PublishCachedLoginActionsAsync in the manual login action flow, invoke RefreshLoginActionsAsync instead of relying only on MonitorLoginActionsSafelyAsync, so pendingLoginRequests is re-fetched even when manifest.LoginActionMonitoringEnabled is disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Steam` Desktop Authenticator/MainForm.cs:
- Around line 125-137: Update FetchTradeConfirmationsForMonitorAsync so the
retryDelay is independent of GetTradeConfirmationMonitorIntervalSeconds and uses
a short bounded backoff, while preserving the existing retryCount of 3 and the
user-configured interval for polling. Keep
GetTradeConfirmationMonitorIntervalSeconds responsible only for the monitor poll
interval.
- Around line 907-918: The trade confirmation cache is populated inconsistently
between the monitor and LoadTradesAsync, allowing auto-confirmed entries to
remain visible after resolution. Update the monitor flow around
CacheTradeConfirmations to cache the same confirmation set as LoadTradesAsync,
or apply the identical auto-confirm filtering in LoadTradesAsync, so both paths
publish matching user-visible entries.
- Around line 2028-2040: Update the WebView2 initialization failure path around
EnsureCoreWebView2Async so monitoring setup is deferred until manifest
initialization completes. Ensure StartBackgroundServicesAfterUiReady or
MainForm_Shown retries both trade and login monitoring after manifest is
available, rather than leaving them disabled for the session.
---
Nitpick comments:
In `@Steam` Desktop Authenticator/MainForm.cs:
- Around line 1624-1625: After PublishCachedLoginActionsAsync in the manual
login action flow, invoke RefreshLoginActionsAsync instead of relying only on
MonitorLoginActionsSafelyAsync, so pendingLoginRequests is re-fetched even when
manifest.LoginActionMonitoringEnabled is disabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 9813aabf-d021-4746-8941-8211dc409c43
📒 Files selected for processing (1)
Steam Desktop Authenticator/MainForm.cs
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Validation
dotnet build SteamDesktopAuthenticator.sln --no-restore -c ReleaseSummary by CodeRabbit
New Features
Bug Fixes
Documentation