Repository navigation
Add login actions and resilient trade monitoring - #10
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe application adds Steam login approval monitoring, manual and automatic decisions, dashboard controls, diagnostic logging, serialized account operations, single-instance activation, and updated authentication library behavior. ChangesLogin and account operations
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Dashboard
participant MainForm
participant LoginApprovalService
participant SteamAuth
User->>Dashboard: Open Login Actions
Dashboard->>MainForm: Load login actions
MainForm->>LoginApprovalService: Fetch pending requests
LoginApprovalService->>SteamAuth: Query sessions and request details
SteamAuth-->>LoginApprovalService: Return pending requests
LoginApprovalService-->>MainForm: Return fetch result
MainForm-->>Dashboard: Render requests
User->>Dashboard: Approve or deny request
Dashboard->>MainForm: Submit decision
MainForm->>LoginApprovalService: RespondAsync
LoginApprovalService->>SteamAuth: Update mobile confirmation
SteamAuth-->>LoginApprovalService: Return action result
LoginApprovalService-->>MainForm: Return success or typed error
MainForm-->>Dashboard: Refresh request state
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
Steam Desktop Authenticator/MainForm.cs (1)
1022-1032: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the login bookkeeping collections.
completedAutomatedLoginActions,notifiedLoginRequests,recordedRecentLoginAttempts,notifiedTradeConfirmations, andautomatedLoginActionNotificationsonly ever grow. Astro SDA runs continuously in the tray, so these sets accumulate one entry per observed request for the lifetime of the process.Add pruning.
automatedLoginActionNotificationsalready carries timestamps, so entries older than the 5-minute suppression window can be dropped. The key sets can be trimmed when a request is reconciled as no longer pending.Also move the
completedAutomatedLoginActionsfield declaration up with the other fields near line 46. A field between method bodies is easy to miss.🤖 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 1022 - 1032, Bound all login bookkeeping collections: move the completedAutomatedLoginActions field declaration to the main field-declaration section near the other fields, prune completedAutomatedLoginActions, notifiedLoginRequests, recordedRecentLoginAttempts, and notifiedTradeConfirmations when their requests are reconciled as no longer pending, and remove automatedLoginActionNotifications entries older than the five-minute suppression window before or during notification handling in NotifyAutomatedLoginActionOnce.
🤖 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 `@lib/SteamAuth/SteamAuth/TimeAligner.cs`:
- Around line 66-79: Guard the deserialized result in GetSteamTimeAsync before
accessing query.Response.ServerTime, treating a null query or response as an
unsuccessful time lookup and returning without updating _timeDifference or
_aligned. Preserve the existing successful alignment behavior and WebException
handling.
In `@Steam` Desktop Authenticator/DiagnosticErrorLogger.cs:
- Around line 94-95: Update the filename-selection logic around the current
File.Exists/FileInfo check to calculate the pending log entry’s UTF-8 byte size
before selecting a file. Accept an existing file only when its current length
plus the entry size remains within MaximumLogFileSizeBytes, and truncate or skip
entries whose own size exceeds the limit so appending cannot exceed the bound.
In `@Steam` Desktop Authenticator/LoginApprovalService.cs:
- Around line 242-252: Update the exception handling around
account.Session.RefreshAccessToken and persistAccount so a failed secure save is
not caught and converted to LoginApprovalErrorKind.SessionExpired. Preserve the
session-expired conversion for refresh failures, while allowing the existing
LoginApprovalException from persistAccount to propagate with its original
message and error kind.
- Around line 315-320: Update FetchPendingRequestsCoreAsync to validate the
sessions response body after ThrowIfSteamFailed and before ParseClientIds by
checking that GetResponseObject(sessionsResponse.Body) returns a non-null
object, matching the existing validation pattern used in
FetchRequestDetailsAsync. Keep ThrowIfSteamFailed as-is, and ensure malformed or
missing response bodies now surface an error instead of falling through to an
empty request list.
In `@Steam` Desktop Authenticator/MainForm.cs:
- Around line 936-946: Update the login monitoring flow around the
LoginApprovalErrorKind.RateLimited handling to apply and honor a cooldown, using
the existing TryGetTradeRateLimitMessage and ApplyTradeRateLimit pattern as
guidance. Skip login monitoring passes while the cooldown is active, apply it
when rate limiting is detected, and record the event through
DiagnosticErrorLogger without changing SessionExpired or other error handling.
- Around line 1990-2014: Update RespondToTradeConfirmationAsync to execute the
AcceptConfirmation or DenyConfirmation call through the existing per-account
serialization mechanism, including confirmationsSemaphore as used by other Steam
confirmation operations. Preserve the existing error handling and
LoadTradesAsync cleanup while ensuring manual responses cannot overlap
confirmation fetching or monitor auto-accept operations for the same account.
- Around line 1906-1914: Update the settings parsing in
CoreWebView2_WebMessageReceived to use nullable boolean casts with false
fallbacks for loginActionMonitoringEnabled and the adjacent boolean payload
fields, including the pre-existing direct casts for checkConfirmations,
checkAllAccounts, autoConfirmMarket, autoConfirmTrades, and minimizeToTray.
Preserve the existing automatic-mode override for LoginActionMonitoringEnabled.
- Around line 906-919: Update loginActionsTimer_Tick and the fire-and-forget
MonitorLoginActionsAsync call in ConfigureLoginActionsMonitor to contain
exceptions from the login monitoring task. Wrap the awaited and discarded
monitor invocations with appropriate exception handling and route failures
through the existing application logging or error-reporting mechanism, ensuring
async-void event execution cannot terminate the application and fire-and-forget
failures are not lost.
In `@Steam` Desktop Authenticator/Manifest.cs:
- Around line 60-61: Change the default initializer of
Manifest.LoginActionMonitoringEnabled to false so existing manifests without the
property remain disabled after deserialization. Preserve GenerateNewManifest’s
explicit true value for fresh installations if that is the intended new-user
behavior.
In `@Steam` Desktop Authenticator/wwwroot/index.html:
- Around line 341-354: Update the recent login attempt modal flow around
showRecentLoginAttempt, closeRecentLoginAttempt, and renderRecentLoginAttempts
to track the button that opened the modal, pass it when showing the modal, and
restore focus to it after closing. Add Escape-key handling for the active dialog
so pressing Escape invokes closeRecentLoginAttempt while preserving the existing
close-button focus behavior.
- Around line 545-555: Update the trade-account change handler that synchronizes
login-account-select to call renderLoginActions() after updating the selector,
matching the existing login selector handler behavior so the Login Actions list
and count reflect the newly selected account.
---
Nitpick comments:
In `@Steam` Desktop Authenticator/MainForm.cs:
- Around line 1022-1032: Bound all login bookkeeping collections: move the
completedAutomatedLoginActions field declaration to the main field-declaration
section near the other fields, prune completedAutomatedLoginActions,
notifiedLoginRequests, recordedRecentLoginAttempts, and
notifiedTradeConfirmations when their requests are reconciled as no longer
pending, and remove automatedLoginActionNotifications entries older than the
five-minute suppression window before or during notification handling in
NotifyAutomatedLoginActionOnce.
🪄 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: 851488bf-f7e2-48cd-8b23-1a30cfb34509
📒 Files selected for processing (14)
.gitignoreSteam Desktop Authenticator/DiagnosticErrorLogger.csSteam Desktop Authenticator/LoginApprovalService.csSteam Desktop Authenticator/LoginForm.csSteam Desktop Authenticator/LoginNotificationPopup.csSteam Desktop Authenticator/MainForm.csSteam Desktop Authenticator/Manifest.csSteam Desktop Authenticator/Program.csSteam Desktop Authenticator/Properties/AssemblyInfo.csSteam Desktop Authenticator/Steam Desktop Authenticator.csprojSteam Desktop Authenticator/wwwroot/index.htmllib/SteamAuth/SteamAuth/SteamGuardAccount.cslib/SteamAuth/SteamAuth/SteamWeb.cslib/SteamAuth/SteamAuth/TimeAligner.cs
Comment on lines
+60
to
+61
| [JsonProperty("login_action_monitoring_enabled")] | ||
| public bool LoginActionMonitoringEnabled { get; set; } = true; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Default login monitoring to disabled for existing manifests.
LoginActionMonitoringEnabled defaults to true. Existing manifests do not contain login_action_monitoring_enabled, so every current installation starts polling Steam every 5 seconds for all accounts after the upgrade, without an explicit opt-in. The UI text in wwwroot/index.html also presents monitoring as a user choice.
Set the default to false, or gate the enable-on-upgrade behavior behind a first-run prompt.
🔧 Proposed change
[JsonProperty("login_action_monitoring_enabled")]
- public bool LoginActionMonitoringEnabled { get; set; } = true;
+ public bool LoginActionMonitoringEnabled { get; set; } = false;GenerateNewManifest at line 158 can keep setting true for fresh installations if that is the intended new-user default.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [JsonProperty("login_action_monitoring_enabled")] | |
| public bool LoginActionMonitoringEnabled { get; set; } = true; | |
| [JsonProperty("login_action_monitoring_enabled")] | |
| public bool LoginActionMonitoringEnabled { get; set; } = false; |
🤖 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/Manifest.cs around lines 60 - 61, Change the
default initializer of Manifest.LoginActionMonitoringEnabled to false so
existing manifests without the property remain disabled after deserialization.
Preserve GenerateNewManifest’s explicit true value for fresh installations if
that is the intended new-user behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
dotnet build SteamDesktopAuthenticator.sln --no-restoreSummary by CodeRabbit
New Features
Bug Fixes
Documentation