fix: prevent updater from panics #1492
Conversation
WalkthroughThis change improves the error handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant UM as UpdatesManager
participant UB as UpdaterBuilder
participant Logger as Logging
UM->>UB: Call updater_builder()
alt Setting endpoints successful
UB-->>UM: Endpoints set
UM->>UB: Build updater
alt Building updater successful
UB-->>UM: Updater built
UM->>UB: Check for updates
alt Update check successful
UB-->>UM: Update info returned
else Update check failure
Logger->>UM: Log warning
UM-->>UM: Return Ok(None)
end
else Building updater failure
Logger->>UM: Log warning
UM-->>UM: Return Ok(None)
end
else Setting endpoints failure
Logger->>UM: Log warning
UM-->>UM: Return Ok(None)
end
Assessment against linked issues
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src-tauri/src/updates_manager.rs (4)
145-154: Consider using a Semantic Versioning comparator rather than plain string comparison.Currently, the closure uses
update.version > current(or!=whenenable_downgrade) to decide if an update is newer. If these versions contain semantic prefixes or follow complex versioning formats, a direct string comparison may yield incorrect conclusions (e.g., "2.10" is lexically less than "2.2"). Consider parsing the versions via a known semver library for more robust comparison.
155-161: Re-examine returningOk(None)upon endpoints error.When the
.endpoints()call fails, you log the error and returnOk(None). This effectively treats "failing to set endpoints" the same as "no updates available." If differentiating these scenarios is useful, consider returning a more descriptive error or a custom type to distinguish between "no updates found" vs. "error setting endpoints."
162-168: Maintain consistency in error reporting or surface the cause.Similar to the previous match, an error in
builder.build()results in a warning and anOk(None). If you need upstream handling or logging to reflect the specific reason, consider returning a dedicated error variant or an error message instead of silently mapping it toNone.
169-175: Ensure callers can differentiate between "no update" and "update check error."Currently, errors from
.check()are also logged as warnings, and the function returnsOk(None). This prevents a panic but conflates genuine 'no update' scenarios with 'failed to check' scenarios. For clarity, you might wrap theNonein a result or provide an explicit error subtype to inform the calling function that an error occurred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src-tauri/src/updates_manager.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: cargo checks (fmt, clippy, check)
- GitHub Check: machete
- GitHub Check: tauri-build
fixes #1491
Description
Throw error instead of letting the app panic when something wrong happened while checking for updates
How Has This Been Tested?
Manually by turning off the wifi
Summary by CodeRabbit