Rust 1.91.1 #167

gr211 · 2025-11-12T22:21:51Z

No description provided.

Copilot

Pull Request Overview

This PR updates the project from Rust 1.90.0 to 1.91.1 (MSRV), bringing numerous code quality improvements including better error handling, simplified code patterns, and dependency updates. The changes focus on making error handling more robust, removing panics in favor of proper error propagation, and modernizing code to use newer Rust idioms.

Updates MSRV to Rust 1.91.1 and dependency versions in Cargo.lock
Improves error handling throughout the codebase with better error messages and reduced panic points
Refactors code for better maintainability (e.g., using Cow<str>, #[from] derives, consolidated imports)

Reviewed Changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
`.github/workflows/rust.yml`	Updates MSRV from 1.90.0 to 1.91.1
`.github/workflows/release.yml`	Updates MSRV from 1.90.0 to 1.91.1
`Cargo.lock`	Comprehensive dependency updates for Rust 1.91.1 compatibility
`src/model/account_group.rs`	Removes unused `ObjectExt` import
`src/model/account_errors.rs`	Improves error messages, consolidates `From` implementations using `#[from]` derive, simplifies `error()` method
`src/model/account.rs`	Refactors TOTP generation with `Cow<str>`, adds error class removal on success, improves error handling
`src/main_window.rs`	Refactors visibility logic into dedicated method, improves error messages, adds mutex poison handling, consolidates imports
`src/main.rs`	Major refactor of startup error handling to avoid panics, improves logging initialization, better resource loading
`src/helpers/qr_code.rs`	Enhances QR code error messages with more context, adds documentation comments
`src/helpers/paths.rs`	Improves error handling with graceful fallbacks, adds mutex poison handling, enhances logging
`src/helpers/mod.rs`	Reorganizes module declarations (declarations before re-exports)
`src/helpers/migrations.rs`	Updates signature to accept `Arc<Mutex<Connection>>` for consistency
`src/helpers/keyring.rs`	Consolidates error handling with new `Result` type alias, removes redundant code, improves consistency
`src/helpers/icon_parser.rs`	Changes selector parsing from panic to graceful failure, improves robustness
`src/helpers/database.rs`	Updates all tests to use `Arc<Mutex<Connection>>` pattern consistently
`src/helpers/backup.rs`	Simplifies serialization/deserialization code, improves error handling, removes test cases
`src/exporting.rs`	Improves dialog handling, adds better error handling for channel operations, uses `.hide()` instead of `.set_visible(false)`

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-12T22:26:20Z

src/main.rs

+            log::error!("Failed to create database connection: {:?}", e);
+            exit(1);


Using log::error! before configure_logging() is called (which happens later at line 80-83) means this error message may not be logged according to the application's logging configuration. The logging system needs to be initialized before any log calls are made.

Additionally, line 29 has a similar issue with eprintln! being used but then line 32 tries to log with info!, which will also fail to work correctly before logging is configured.

Copilot · 2025-11-12T22:26:21Z

src/main_window.rs

-                    widgets.iter_mut().for_each(|group| group.update());
+
+                // Update TOTP widgets on 0 and 30 seconds (every 30s)
+                if seconds.is_multiple_of(30) {


Using is_multiple_of(30) is less clear than the original seconds == 0 || seconds == 30 because it obscures the intent. For second values in range 0-59, both are equivalent, but the explicit comparison makes it immediately obvious that updates happen at 0 and 30 seconds. The new approach requires mental calculation to understand when it triggers.

Consider: Reverting to seconds == 0 || seconds == 30 for better code clarity.

Suggested change

if seconds.is_multiple_of(30) {

if seconds == 0 || seconds == 30 {

Copilot · 2025-11-12T22:26:22Z

src/helpers/keyring.rs

        let str_account_id = format!("{}", account_id);
+        let mut attributes: HashMap<&str, &str> = HashMap::new();
        attributes.insert(ACCOUNT_ID_KEY, str_account_id.as_str());
        attributes.insert(APPLICATION_KEY, APPLICATION_VALUE);


The error message "database connection mutex was poisoned" uses io::Error::other() which requires Rust 1.91+. While this matches the MSRV update in the PR, it would be better to use a more specific error type from RepositoryError rather than wrapping it as an IO error, since a poisoned mutex is not truly an I/O error.

Consider: Adding a dedicated RepositoryError::MutexPoisoned variant for better error semantics.

Copilot · 2025-11-12T22:26:22Z

src/helpers/backup.rs

-        let mut accounts = Self::deserialise_accounts(path.as_path())?;
+        let mut account_groups = Self::deserialise_accounts(path.as_path())?;

        let connection = connection.lock().unwrap();


Using .unwrap() on the mutex lock in production code. This is inconsistent with the approach taken in paths.rs line 65-67 where poisoned mutexes are handled gracefully. If the mutex is poisoned, this will panic.

Consider: Adding proper error handling for poisoned mutex, similar to the pattern in paths.rs.

Suggested change

let connection = connection.lock().unwrap();

let connection = match connection.lock() {

Ok(conn) => conn,

Err(poisoned) => {

warn!("Database connection mutex was poisoned. Recovering.");

poisoned.into_inner()

}

};

Copilot · 2025-11-12T22:26:22Z

src/helpers/backup.rs

-        let result = QrCode::process_qr_code(path.to_str().unwrap().to_owned()).await;
+        let path_str = path
+            .to_str()
+            .ok_or_else(|| RepositoryError::IoError(io::Error::new(io::ErrorKind::InvalidInput, "invalid unicode in path")))?;


The error message created here uses io::Error::new(io::ErrorKind::InvalidInput, ...) which is good, but the error message "invalid unicode in path" could be more helpful by indicating what operation failed.

Consider: "Failed to convert path to string: invalid unicode" for better context.

Suggested change

.ok_or_else(|| RepositoryError::IoError(io::Error::new(io::ErrorKind::InvalidInput, "invalid unicode in path")))?;

.ok_or_else(|| RepositoryError::IoError(io::Error::new(io::ErrorKind::InvalidInput, "Failed to convert path to string: invalid unicode")))?;

Copilot · 2025-11-12T22:26:23Z

src/model/account.rs

+            return key;
        }
+        let pad = 32 - key.len();
+        key.to_mut().push_str(&"=".repeat(pad).to_string());


Redundant .to_string() call. The repeat() method already returns a String, so calling .to_string() on it is unnecessary and creates an extra allocation.

Suggestion: Change to key.to_mut().push_str(&"=".repeat(pad));

Suggested change

key.to_mut().push_str(&"=".repeat(pad).to_string());

key.to_mut().push_str(&"=".repeat(pad));

Copilot · 2025-11-12T22:26:24Z

src/model/account.rs

+        let secret = base32::decode(Alphabet::Rfc4648 { padding: true }, &padded).ok_or_else(|| TotpError::InvalidKey(key.to_string()))?;

+        let totp_sha1 = totp_rs::TOTP::new(totp_rs::Algorithm::SHA1, 6, 1, 30, secret)?;
        totp_sha1.generate_current().map_err(TotpError::SystemTimeError)


The error conversion here discards the original SystemTimeError. Since TotpError::SystemTimeError already has a #[from] attribute that handles conversion, this explicit map_err is redundant and loses the automatic conversion benefit.

Suggestion: Change to totp_sha1.generate_current()? to use the automatic conversion from the #[from] attribute.

Suggested change

totp_sha1.generate_current().map_err(TotpError::SystemTimeError)

totp_sha1.generate_current()?

Copilot · 2025-11-12T22:26:24Z

src/helpers/migrations.rs

    #[allow(clippy::result_large_err)]
-    pub fn run(connection: &mut Connection) -> Result<Report, Error> {
+    pub fn run(connection: Arc<Mutex<Connection>>) -> Result<Report, Error> {
+        let mut connection = connection.lock().unwrap();


Using .unwrap() on a mutex lock can cause a panic if the mutex is poisoned. Given that this is in a migration function that runs during startup, and other parts of the codebase now handle poisoned mutexes gracefully (e.g., in paths.rs line 67), this should also handle the poisoned mutex case.

Suggestion: Use .map_err() to convert the poison error into a refinery Error or another appropriate error type.

Suggested change

let mut connection = connection.lock().unwrap();

let mut connection = connection

.lock()

.map_err(|e| Error::Other(format!("Mutex poisoned while running migrations: {}", e)))?;

Copilot · 2025-11-12T22:26:25Z

src/helpers/icon_parser.rs

+            let selector_1 = Selector::parse(r#"link[rel="apple-touch-icon"]"#).ok();
+            let selector_2 = Selector::parse(r#"link[rel="shortcut icon"]"#).ok();
+            let selector_3 = Selector::parse(r#"link[rel="icon"]"#).ok();

-            let option_1 = document.select(&selector_1).next();
-            let option_2 = document.select(&selector_2).next();
-            let option_3 = document.select(&selector_3).next();
+            let option_1 = selector_1.and_then(|s| document.select(&s).next());
+            let option_2 = selector_2.and_then(|s| document.select(&s).next());
+            let option_3 = selector_3.and_then(|s| document.select(&s).next());


The change from .unwrap() to .ok() followed by .and_then() is good for avoiding panics. However, if selector parsing fails (which should never happen with these hardcoded selector strings), the code silently returns no match. While this is acceptable defensive programming, the original .unwrap() would catch programmer errors during development.

This is an acceptable tradeoff, but consider adding debug assertions or comments explaining that these selector strings are known-valid.

Copilot · 2025-11-12T22:26:25Z

src/helpers/backup.rs


                let mut account_groups = AccountGroup::new(0, "GAuth", None, None, false, entries);

                let connection = connection.lock().unwrap();


Using .unwrap() on the mutex lock in production code. This should handle poisoned mutex errors gracefully, consistent with the error handling improvements made elsewhere in this PR (e.g., paths.rs line 65-67).

Suggested change

let connection = connection.lock().unwrap();

let connection = connection.lock().map_err(|e| RepositoryError::MutexPoisonError(format!("{}", e)))?;

gr211 added 20 commits November 12, 2025 21:24

1.91.1

7942332

1.91.1

26e410c

1.91.1

b9f5f89

1.91.1

a34d916

1.91.1

ac5c348

1.91.1

6a5a3f8

1.91.1

18cdc08

1.91.1

07a1357

1.91.1

820084d

1.91.1

f437883

1.91.1

70e062f

1.91.1

cc54b2c

1.91.1

2b7ece1

1.91.1

506c43c

1.91.1

cfa4e3c

1.91.1

b4331f1

1.91.1

7e50b2c

1.91.1

bea4e15

1.91.1

91a4b31

1.91.1

761e9e6

gr211 self-assigned this Nov 12, 2025

gr211 requested a review from Copilot November 12, 2025 22:21

Copilot started reviewing on behalf of gr211 November 12, 2025 22:22 View session

Copilot finished reviewing on behalf of gr211 November 12, 2025 22:25

Copilot AI reviewed Nov 12, 2025

View reviewed changes

gr211 added 5 commits November 12, 2025 23:31

1.91.1

0c2676e

1.91.1

e8bf1e3

1.91.1

b714393

1.91.1

e67c1dd

1.91.1

e3003f0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Rust 1.91.1 #167

Rust 1.91.1 #167

Uh oh!

gr211 commented Nov 12, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		log::error!("Failed to create database connection: {:?}", e);
		exit(1);

	if seconds.is_multiple_of(30) {
	if seconds == 0 \|\| seconds == 30 {

-        let connection = connection.lock().unwrap();
+        let connection = match connection.lock() {
+            Ok(conn) => conn,
+            Err(poisoned) => {
+                warn!("Database connection mutex was poisoned. Recovering.");
+                poisoned.into_inner()
+            }
+        };

	.ok_or_else(\|\| RepositoryError::IoError(io::Error::new(io::ErrorKind::InvalidInput, "invalid unicode in path")))?;
	.ok_or_else(\|\| RepositoryError::IoError(io::Error::new(io::ErrorKind::InvalidInput, "Failed to convert path to string: invalid unicode")))?;

	key.to_mut().push_str(&"=".repeat(pad).to_string());
	key.to_mut().push_str(&"=".repeat(pad));

	totp_sha1.generate_current().map_err(TotpError::SystemTimeError)
	totp_sha1.generate_current()?


		let mut account_groups = AccountGroup::new(0, "GAuth", None, None, false, entries);

		let connection = connection.lock().unwrap();

	let connection = connection.lock().unwrap();
	let connection = connection.lock().map_err(\|e\| RepositoryError::MutexPoisonError(format!("{}", e)))?;

Rust 1.91.1 #167

Are you sure you want to change the base?

Rust 1.91.1 #167

Uh oh!

Conversation

gr211 commented Nov 12, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants