Open
Conversation
Make WASM auto-login behave the same as desktop: retrieve credentials immediately when GMCP negotiation completes, without requiring a click. Defer getPassword() via QTimer::singleShot(0) to avoid ASYNCIFY stack overflow from deep within the GMCP signal chain. Improve Preferences UI: disable credential fields when "Remember my login" is unchecked, clear stored credentials on uncheck, show dummy dots for stored passwords, and delete stale IndexedDB records when the password field is cleared.
Reviewer's GuideImplements WASM-specific encrypted password storage and auto-login behavior aligned with desktop, refines the Preferences auto-login UI and behavior (including deletion of stored credentials), fixes WASM auto-login and double-input issues, and adjusts build/test configuration for newer Qt/GLM, QtKeychain deployment, and immer fetching. Sequence diagram for WASM auto-login password retrieval via deferred GMCP handlingsequenceDiagram
participant GMCPHandler
participant Proxy
participant PasswordConfig
participant QTimer
participant WasmPasswordBackend
participant BrowserIndexedDB
GMCPHandler->>Proxy: GMCP negotiation completed
Proxy->>Proxy: allocMudTelnet()
Proxy->>Proxy: check rememberLogin && accountName && accountPassword
alt Platform_Wasm
Proxy->>QTimer: singleShot(0, Proxy, lambda)
QTimer-->>Proxy: invoke lambda on next event loop
Proxy->>PasswordConfig: getPassword()
PasswordConfig->>WasmPasswordBackend: wasm_read_password(WASM_PASSWORD_KEY)
WasmPasswordBackend->>BrowserIndexedDB: open mmapper-credentials/passwords
BrowserIndexedDB-->>WasmPasswordBackend: stored record or none
alt record_found
WasmPasswordBackend-->>PasswordConfig: malloc'd UTF8 password
PasswordConfig-->>Proxy: sig_incomingPassword(QString)
else no_record
WasmPasswordBackend-->>PasswordConfig: empty string
PasswordConfig-->>Proxy: no signal (silent)
end
else Other_platforms_with_QtKeychain
Proxy->>PasswordConfig: getPassword()
PasswordConfig->>QtKeychain_ReadJob: start()
QtKeychain_ReadJob-->>PasswordConfig: finished with password or error
PasswordConfig-->>Proxy: sig_incomingPassword or sig_error
end
Class diagram for updated PasswordConfig and GeneralPage auto-login behaviorclassDiagram
class PasswordConfig {
+PasswordConfig(QObject *parent)
+void setPassword(QString password)
+void getPassword()
+void deletePassword()
+void sig_error(QString msg)
+void sig_incomingPassword(QString password)
}
class GeneralPage {
+GeneralPage(QWidget *parent)
-Ui_GeneralPage *ui
-PasswordConfig passCfg
-bool m_passwordFieldHasDummy
+void slot_loadConfig()
}
class Configuration_AccountSettings {
+QString accountName
+bool accountPassword
+bool rememberLogin
+void read(QSettings conf)
}
class Proxy {
+void allocMudTelnet()
+PasswordConfig~ getPasswordConfig()
}
class WasmPasswordBackend {
+void wasm_store_password(char *key, char *password)
+char * wasm_read_password(char *key)
+void wasm_delete_password(char *key)
<<WASM JS bridge>>
}
GeneralPage --> PasswordConfig : owns
GeneralPage --> Configuration_AccountSettings : uses_config
Proxy --> PasswordConfig : uses
PasswordConfig --> WasmPasswordBackend : calls_on_WASM
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In the WASM
getPassword()implementation,wasm_read_passwordreturning null is treated as an error and emitssig_error, which means a missing entry (e.g. no password ever saved or already deleted) is reported the same as an actual storage failure; consider distinguishing "not found" from real errors so normal states don’t show as failures. - The
m_passwordFieldHasDummyflag is set/reset in several places but is never used to alter behavior (e.g. clearing dummy dots on first real edit), so either wire it into the password field logic or remove it to avoid dead state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the WASM `getPassword()` implementation, `wasm_read_password` returning null is treated as an error and emits `sig_error`, which means a missing entry (e.g. no password ever saved or already deleted) is reported the same as an actual storage failure; consider distinguishing "not found" from real errors so normal states don’t show as failures.
- The `m_passwordFieldHasDummy` flag is set/reset in several places but is never used to alter behavior (e.g. clearing dummy dots on first real edit), so either wire it into the password field logic or remove it to avoid dead state.
## Individual Comments
### Comment 1
<location path="src/configuration/PasswordConfig.cpp" line_range="67-57" />
<code_context>
+EM_ASYNC_JS(char *, wasm_read_password, (const char *key), {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `wasm_read_password` conflates not-found and error cases, which makes caller logic treat both as errors.
The function currently returns `0` both when no record exists and when an exception occurs. On the C++ side, `getPassword()` treats any resulting `nullptr` as `sig_error("Failed to retrieve password from browser storage.")`, so a normal “no password stored yet” case is surfaced as an error. To allow correct handling (e.g., quiet UX and different flows for “not found” vs. real failure), return a distinct status or sentinel value so the C++ side can differentiate the two cases.
Suggested implementation:
```cpp
// Read and decrypt a password from IndexedDB.
// Returns:
// - malloc'd UTF-8 string with the password on success
// - malloc'd empty string ("") when no record exists (not found)
// - NULL on unexpected error
// Caller must free() the returned pointer when non-NULL.
EM_ASYNC_JS(char *, wasm_read_password, (const char *key), {
```
To fully implement the distinction between "not found" and "error", you should also:
1. **Inside `wasm_read_password` (JS body in the same file):**
- Locate the code that currently handles the "no record" case, which likely looks similar to:
```js
if (!result) {
return 0;
}
```
or:
```js
if (getReq.result === undefined) {
return 0;
}
```
- Replace that "not found" branch with something that returns an allocated empty string:
```js
if (getReq.result === undefined) {
// Not found: return a malloc'd empty string so C++ can distinguish it.
const emptyPtr = _malloc(1);
HEAPU8[emptyPtr] = 0; // null terminator
return emptyPtr;
}
```
- Keep returning `0` (`NULL`) only for genuine error paths (e.g., inside `catch (e)`).
2. **On the C++ side where `wasm_read_password` is consumed (likely in `PasswordConfig::getPassword()`):**
- After calling `wasm_read_password`, distinguish the three cases:
```cpp
char* raw = wasm_read_password(key.c_str());
if (!raw) {
// NULL => real failure
sig_error("Failed to retrieve password from browser storage.");
}
if (raw[0] == '\0') {
// Empty string => "no password stored yet" (not found).
// Handle this as the benign "not configured" case, e.g.:
// - quiet UX
// - alternative flow
// - ensure you still free(raw)
std::string password; // empty => not found
free(raw);
return password;
}
// Normal success path with a non-empty password
std::string password(raw);
free(raw);
return password;
```
- Ensure all call sites that currently treat `nullptr` as “no password stored yet” are updated to interpret **empty string** as “not found” and **`nullptr`** as “real error”.
3. **Update any related documentation or comments** that still state "NULL on error/not found" to reflect the new tri-state behavior (success / not-found / error) to keep future changes correct.
</issue_to_address>
### Comment 2
<location path="src/preferences/generalpage.cpp" line_range="23-32" />
<code_context>
+class WasmInputDeduplicateFilter final : public QObject
</code_context>
<issue_to_address>
**suggestion:** The input deduplication filter may interfere with more complex input methods or multi-character commits.
This assumes a 1:1 mapping between `QKeyEvent::text()` and `QInputMethodEvent::commitString()`. For IME/composed input (especially on WASM), `commitString()` can contain multiple characters or differ from the key event text, so the `m_lastText == text/commit` heuristic could suppress valid commits. Consider tightening the suppression criteria (e.g., only for single printable characters with matching key/modifier state) or limiting this filter to contexts where simple Latin input is expected, to avoid breaking IME-heavy languages.
Suggested implementation:
```cpp
if (event->type() == QEvent::KeyPress) {
auto *ke = static_cast<QKeyEvent *>(event);
const QString text = ke->text();
// Only consider simple single-character, printable key events without
// Ctrl/Alt/Meta modifiers for deduplication. This avoids interfering
// with IMEs and composed/multi-character input where commitString()
// may not have a 1:1 mapping with QKeyEvent::text().
const bool isSimpleChar =
text.size() == 1 &&
!text.at(0).isControl() &&
!(ke->modifiers() & (Qt::ControlModifier |
Qt::AltModifier |
Qt::MetaModifier));
if (!isSimpleChar) {
// Do not track this key event for deduplication – it likely
// belongs to a more complex input method. Clear last text so that
// subsequent QInputMethodEvent commits are not accidentally
// suppressed.
m_lastText.clear();
return QObject::eventFilter(obj, event);
}
```
Because only part of the class is visible, you should also adjust the `QInputMethodEvent` handling inside `WasmInputDeduplicateFilter`:
1. When you decide whether to suppress the `QInputMethodEvent`, require that:
- `event->type() == QEvent::InputMethod`,
- `const auto *ime = static_cast<QInputMethodEvent *>(event);`
- `const QString commit = ime->commitString();`
- `commit.size() == 1` and `!commit.at(0).isControl()`,
- and `commit == m_lastText`.
Only in that case should you return `true` (to suppress the duplicate commit). For any other `commitString()` (empty, multi-character, or not matching `m_lastText`), allow the event to pass through (return `false`).
2. Ensure `m_lastText` is set only in the “simple character” `QKeyEvent` path (after the block shown above) and cleared when appropriate (e.g. after a successful deduplication or when a non-deduplicated input method event is seen).
3. If there is any logic that currently suppresses on `m_lastText == text/commit` without these extra checks, tighten it to use the criteria described above, so IME-heavy languages and multi-character commits are not affected.
</issue_to_address>
### Comment 3
<location path="src/clock/mumeclock.cpp" line_range="81-83" />
<code_context>
public:
NODISCARD static int keyToValue(const QString &key) { return g_qme.keyToValue(key.toUtf8()); }
- NODISCARD static QString valueToKey(const int value) { return g_qme.valueToKey(value); }
+ NODISCARD static QString valueToKey(const int value)
+ {
+ return g_qme.valueToKey(static_cast<quint64>(value));
+ }
NODISCARD static int keyCount() { return g_qme.keyCount(); }
</code_context>
<issue_to_address>
**issue (bug_risk):** Casting `int` to `quint64` in `valueToKey` could mis-handle negative values or out-of-range inputs.
The previous version passed the `int` directly; now it’s converted to `quint64` first. If `value` is ever negative or otherwise out of range, this will be reinterpreted as a large unsigned integer and cause incorrect lookups. If negatives are impossible, consider enforcing that via an assertion or a `quint64` parameter; otherwise, add validation or clamping before casting.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #468 +/- ##
===========================================
- Coverage 66.48% 25.76% -40.72%
===========================================
Files 85 501 +416
Lines 4186 41466 +37280
Branches 255 4515 +4260
===========================================
+ Hits 2783 10684 +7901
- Misses 1403 30782 +29379 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…erialize a non-extractable CryptoKey object via the structured clone algorithm. Chrome handles this fine, Firefox doesn't.
Fix:
1. wasm_store_password: Changed extractable from false to true, then exports the key as raw bytes (crypto.subtle.exportKey("raw", ...)) and stores the Uint8Array instead of the CryptoKey object. Only plain byte
arrays (rawKey, iv, ciphertext) go into IndexedDB now — no browser-specific serialization issues.
2. wasm_read_password: Imports the raw key bytes back into a CryptoKey (crypto.subtle.importKey("raw", ...)) before decrypting. The imported key is non-extractable since it's only needed for the decrypt operation.
Security impact: None meaningful. The password is already available as plaintext in JS memory during encryption/decryption. The encryption protects at-rest data in IndexedDB — storing raw key bytes alongside the
ciphertext in the same record is equivalent to storing the CryptoKey object.
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
m_passwordFieldHasDummyflag inGeneralPageis written in several places but never read, so it can either be removed or wired into logic that actually depends on the dummy-password state to avoid dead state and future confusion. - In the WASM password storage EM_JS helpers, you are storing the AES-GCM raw key alongside the ciphertext and IV in IndexedDB, which largely defeats the purpose of encryption; consider either simplifying to plaintext storage or moving the key material to a different, better-protected location (or deriving it) so the added complexity has a security benefit.
- The IndexedDB open/upgrade/transaction logic is duplicated across
wasm_store_password,wasm_read_password, andwasm_delete_password; factoring this into a shared JS helper (e.g., a small internal module onModule) would make future schema changes or error handling tweaks less error-prone.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `m_passwordFieldHasDummy` flag in `GeneralPage` is written in several places but never read, so it can either be removed or wired into logic that actually depends on the dummy-password state to avoid dead state and future confusion.
- In the WASM password storage EM_JS helpers, you are storing the AES-GCM raw key alongside the ciphertext and IV in IndexedDB, which largely defeats the purpose of encryption; consider either simplifying to plaintext storage or moving the key material to a different, better-protected location (or deriving it) so the added complexity has a security benefit.
- The IndexedDB open/upgrade/transaction logic is duplicated across `wasm_store_password`, `wasm_read_password`, and `wasm_delete_password`; factoring this into a shared JS helper (e.g., a small internal module on `Module`) would make future schema changes or error handling tweaks less error-prone.
## Individual Comments
### Comment 1
<location path="src/preferences/generalpage.cpp" line_range="226-234" />
<code_context>
+ }
});
connect(ui->showPassword, &QAbstractButton::clicked, this, [this]() {
if (ui->showPassword->text() == "Hide Password") {
+ // Hide: restore dummy dots in password mode
ui->showPassword->setText("Show Password");
- ui->accountPassword->clear();
ui->accountPassword->setEchoMode(QLineEdit::Password);
- } else if (getConfig().account.accountPassword && ui->accountPassword->text().isEmpty()) {
- ui->showPassword->setText("Request Password");
+ if (getConfig().account.accountPassword) {
+ const QSignalBlocker blocker(ui->accountPassword);
+ ui->accountPassword->setText(QString(8, QChar(0x2022)));
+ m_passwordFieldHasDummy = true;
+ } else {
+ ui->accountPassword->clear();
</code_context>
<issue_to_address>
**issue (bug_risk):** The m_passwordFieldHasDummy flag is written in several places but never read, which suggests either dead state or a missing branch that should consult it.
This flag is set in multiple places (auto-login off, password textEdited, showPassword toggle, slot_loadConfig) but never consulted to affect behavior, so it’s effectively dead state. If it’s meant to distinguish dummy bullets from a real password, that logic is unfinished. Either use the flag in the show/hide/retrieval flow where appropriate, or remove it to avoid misleading state.
</issue_to_address>
### Comment 2
<location path="src/configuration/PasswordConfig.cpp" line_range="25-34" />
<code_context>
+EM_JS(void, wasm_store_password, (const char *key, const char *password), {
</code_context>
<issue_to_address>
**🚨 question (security):** Storing the AES key alongside the ciphertext in IndexedDB weakens the value of encrypting the password at rest.
The current JS flow generates an AES-GCM key, encrypts the password, then stores the raw key, IV, and ciphertext together in IndexedDB. Anyone who can read that IndexedDB entry can decrypt the password, so this only guards against accidental leaks, not a determined attacker. If you want protection from other contexts/extensions in the same profile, consider using a non-extractable Web Crypto CryptoKey (and solving Firefox’s DataCloneError differently), or document this trade-off and threat model explicitly in a comment.
</issue_to_address>
### Comment 3
<location path="tests/TestWasmInputFilter.cpp" line_range="79" />
<code_context>
+ QCOMPARE(edit.text(), QString("a"));
+}
+
+void TestWasmInputFilter::testDifferentCharsNotSuppressed()
+{
+ QLineEdit edit;
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for multi-character commit strings (IME / paste) to ensure the filter does not over-suppress input.
Current tests cover only single-character input. Since `QInputMethodEvent::commitString()` can include multiple characters (IME, paste, autocomplete), please add tests for multi-character commits, for example:
* A `KeyPress` for a single character followed by an `InputMethod` with a longer commit string (e.g. "ab"), asserting that only true duplicates are suppressed.
* Two `InputMethod` events with multi-character commits (e.g. "あ" then "い") to confirm they are not incorrectly filtered.
This will verify the filter behaves correctly for realistic IME/paste scenarios on WASM.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
SourceryAI
reviewed
Mar 1, 2026
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The password visibility toggle logic in
GeneralPagerelies on hard-coded button text values ("Show Password"/"Hide Password"), which is fragile and bypasses any future localization; consider tracking the state with an internal boolean or the line edit’sechoModeinstead of string comparisons. - The WASM password storage helpers assume
crypto.subtleand IndexedDB are available; you might want to guard these with capability checks and surface a clearer error path (e.g., a specific error signal) when running in environments that lack these APIs. - The
AccountSettings::readlogic forrememberLoginusing(NO_QTKEYCHAIN && CURRENT_PLATFORM != PlatformEnum::Wasm) ? false : ...is a bit hard to parse; consider refactoring into named constexprs or an explicitifto make the platform/QtKeychain conditions more readable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The password visibility toggle logic in `GeneralPage` relies on hard-coded button text values ("Show Password"/"Hide Password"), which is fragile and bypasses any future localization; consider tracking the state with an internal boolean or the line edit’s `echoMode` instead of string comparisons.
- The WASM password storage helpers assume `crypto.subtle` and IndexedDB are available; you might want to guard these with capability checks and surface a clearer error path (e.g., a specific error signal) when running in environments that lack these APIs.
- The `AccountSettings::read` logic for `rememberLogin` using `(NO_QTKEYCHAIN && CURRENT_PLATFORM != PlatformEnum::Wasm) ? false : ...` is a bit hard to parse; consider refactoring into named constexprs or an explicit `if` to make the platform/QtKeychain conditions more readable.
## Individual Comments
### Comment 1
<location path="src/configuration/PasswordConfig.cpp" line_range="41-50" />
<code_context>
+ var rawKey = new Uint8Array(await crypto.subtle.exportKey("raw", cryptoKey));
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Storing the raw AES key next to the ciphertext largely nullifies the benefit of encryption at rest in IndexedDB.
Because `rawKey`, `iv`, and `ciphertext` are stored together in the same record, any script with access to this origin’s IndexedDB can decrypt the password, so this is closer to obfuscation than true at‑rest encryption. If you need protection against XSS or similar, consider deriving the key from a user secret (e.g., passphrase) or a non-persisted per‑session key so it can’t be recovered from storage alone. Otherwise, please document this tradeoff clearly and centralize the crypto parameters (algorithm, key length, IV size) so they can be revised more easily later.
Suggested implementation:
```cpp
);
// NOTE:
// This encryption is intended to prevent casual inspection of stored passwords
// in IndexedDB. Because any script with access to this origin can also access
// IndexedDB, this does NOT protect against XSS or other in-origin attackers.
// The AES parameters are centralized here so they can be revised more easily later.
const AES_GCM_PARAMS = { name: "AES-GCM" };
const AES_GCM_IV_LENGTH_BYTES = 12;
var iv = crypto.getRandomValues(new Uint8Array(AES_GCM_IV_LENGTH_BYTES));
var encoded = new TextEncoder().encode(passwordStr);
var ciphertext = await crypto.subtle.encrypt(
{ ...AES_GCM_PARAMS, iv: iv },
cryptoKey,
encoded
);
```
1. Ensure that wherever the encrypted password record is written to IndexedDB, only `iv` and `ciphertext` (and any needed metadata) are stored; remove any reference to `rawKey` or the exported key material from the stored record.
2. Update the corresponding decryption logic to obtain or derive `cryptoKey` without reading it from IndexedDB (e.g., reusing an in-memory per-session key or deriving it from a user secret, depending on your threat model).
3. If stronger protection against XSS or origin-compromising attackers is required, introduce a key derivation step (e.g., using PBKDF2 or WebCrypto’s HKDF) from a user-supplied passphrase, and document that behavior near this code and in your higher-level documentation.
</issue_to_address>
### Comment 2
<location path="src/proxy/proxy.cpp" line_range="517-508" />
<code_context>
+ // We capture `this` (LocalMudTelnetOutputs) which is safe because
+ // LocalMudTelnetOutputs is owned by the Proxy's pipeline, and the
+ // Proxy receiver guard prevents the lambda from firing after destruction.
+ QTimer::singleShot(0, &getProxy(), [this]() {
+ getProxy().getPasswordConfig().getPassword();
+ });
+ } else {
</code_context>
<issue_to_address>
**issue (bug_risk):** Capturing `this` in the deferred lambda risks a use-after-free if `LocalMudTelnetOutputs` is destroyed before the timer fires.
`QTimer::singleShot` being tied to `Proxy` only guarantees the `Proxy` stays alive, not `LocalMudTelnetOutputs`. If `LocalMudTelnetOutputs` is destroyed while `Proxy` remains, the lambda can still run and dereference a dangling `this`. Instead, either avoid capturing `this` by only using data guaranteed to outlive the timer, capture a `QPointer<LocalMudTelnetOutputs>` and check it before use, or emit a signal and handle this logic in a slot so Qt manages lifetimes safely.
</issue_to_address>
### Comment 3
<location path="src/preferences/generalpage.cpp" line_range="214-223" />
<code_context>
});
connect(ui->accountPassword, &QLineEdit::textEdited, this, [this](const QString &password) {
+ if (m_passwordFieldHasDummy) {
+ m_passwordFieldHasDummy = false;
+ // User started typing over dummy dots — clear and keep only the new character.
+ // The last character is the one just typed; the rest are bullet placeholders.
+ const QSignalBlocker blocker(ui->accountPassword);
+ const QString newChar = password.right(1);
+ ui->accountPassword->setText(newChar);
+ setConfig().account.accountPassword = !newChar.isEmpty();
+ if (!newChar.isEmpty()) {
+ passCfg.setPassword(newChar);
+ }
+ return;
+ }
setConfig().account.accountPassword = !password.isEmpty();
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Overwriting the stored password with the first typed character when editing dummy dots may surprise users and lose the original password prematurely.
Because `m_passwordFieldHasDummy` is set, the first edit clears the field and immediately calls `passCfg.setPassword(newChar)`, replacing the stored password with a single character while the user is still typing. If the app crashes or the user navigates away before finishing and saving, the original password is lost. Consider deferring `setPassword` until the edit is complete (e.g., on focus-out/Enter), or at least buffer the full new password locally and only update once there is a confirmed non-empty value.
Suggested implementation:
```cpp
connect(ui->accountPassword, &QLineEdit::textEdited, this, [this](const QString &password) {
if (m_passwordFieldHasDummy) {
m_passwordFieldHasDummy = false;
// User started typing over dummy dots — clear and keep only the new character.
// The last character is the one just typed; the rest are bullet placeholders.
const QSignalBlocker blocker(ui->accountPassword);
const QString newChar = password.right(1);
ui->accountPassword->setText(newChar);
// Update the "has password" flag based on the current field content,
// but do not overwrite the stored password yet.
setConfig().account.accountPassword = !newChar.isEmpty();
return;
}
setConfig().account.accountPassword = !password.isEmpty();
if (!password.isEmpty()) {
passCfg.setPassword(password);
} else {
passCfg.deletePassword();
}
});
```
If you want to go further and only persist the new password once editing is "complete", you can:
1. Connect to `ui->accountPassword`'s `editingFinished` signal and, in that handler, call `passCfg.setPassword(ui->accountPassword->text())` when non-empty, or `passCfg.deletePassword()` when empty.
2. In that case, you should remove or adjust the `passCfg.setPassword(...)` and `passCfg.deletePassword()` calls from the `textEdited` handler to avoid redundant writes and keep all persistence in the "edit complete" path.
</issue_to_address>Hi @KasparMetsa! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In the WASM password EM_JS helpers, the IndexedDB open/upgrade logic is duplicated across store/read/delete; consider factoring this into a shared JS helper to keep the DB name/store/version handling consistent and easier to maintain.
- wasm_read_password() does not participate in the Module._passwordSaveQueue sequencing used by wasm_store_password()/wasm_delete_password(), so reads can race with in-flight writes/deletes; if ordering matters, consider routing reads through the same promise chain or an explicit queue.
- In GeneralPage::GeneralPage(), the accountPassword textEdited handler treats the first edit over dummy dots as a single new character (password.right(1)), which will drop all but the last character if the user pastes or types multiple characters at once; you may want to preserve the full new input instead of truncating it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the WASM password EM_JS helpers, the IndexedDB open/upgrade logic is duplicated across store/read/delete; consider factoring this into a shared JS helper to keep the DB name/store/version handling consistent and easier to maintain.
- wasm_read_password() does not participate in the Module._passwordSaveQueue sequencing used by wasm_store_password()/wasm_delete_password(), so reads can race with in-flight writes/deletes; if ordering matters, consider routing reads through the same promise chain or an explicit queue.
- In GeneralPage::GeneralPage(), the accountPassword textEdited handler treats the first edit over dummy dots as a single new character (password.right(1)), which will drop all but the last character if the user pastes or types multiple characters at once; you may want to preserve the full new input instead of truncating it.
## Individual Comments
### Comment 1
<location path="src/preferences/generalpage.cpp" line_range="213-219" />
<code_context>
});
connect(ui->accountPassword, &QLineEdit::textEdited, this, [this](const QString &password) {
+ if (m_passwordFieldHasDummy) {
+ m_passwordFieldHasDummy = false;
+ // User started typing over dummy dots — clear and keep only the new character.
+ // The last character is the one just typed; the rest are bullet placeholders.
+ const QSignalBlocker blocker(ui->accountPassword);
+ const QString newChar = password.right(1);
+ ui->accountPassword->setText(newChar);
+ setConfig().account.accountPassword = !newChar.isEmpty();
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `password.right(1)` when overwriting the dummy bullets breaks multi-character edits (e.g., paste) for the first edit.
Because `m_passwordFieldHasDummy` assumes the first `textEdited` is a single keystroke and keeps only `password.right(1)`, any multi-character edit (paste, IME, etc.) will be truncated to the last character, and the stored password will briefly be that single character.
In this branch, consider treating `password` as the full new value (removing the dummy bullets but preserving all characters), e.g.:
```cpp
const QSignalBlocker blocker(ui->accountPassword);
ui->accountPassword->setText(password);
passCfg.setPassword(password);
```
`QSignalBlocker` should still prevent re-entry loops.
</issue_to_address>
### Comment 2
<location path="src/preferences/generalpage.cpp" line_range="235-244" />
<code_context>
+ connect(ui->showPassword, &QAbstractButton::clicked, this, [this]() {
</code_context>
<issue_to_address>
**suggestion:** The show/hide logic doesn’t allow revealing a password that exists only in the current text field (not persisted).
`showPassword` currently handles only two cases: toggling back to hidden when the button text is "Hide Password", and calling `passCfg.getPassword()` when `getConfig().account.accountPassword` is true. When `account.accountPassword` is false but the user has typed a password into `accountPassword`, clicking the button does nothing. If users should be able to reveal what they’ve just typed even before it’s persisted, consider handling the case where the field is non-empty but no stored password exists by toggling the echo mode directly instead of calling `getPassword()`.
Suggested implementation:
```cpp
connect(ui->showPassword, &QAbstractButton::clicked, this, [this]() {
const bool hasStoredPassword = getConfig().account.accountPassword;
const bool hasFieldText = !ui->accountPassword->text().isEmpty();
if (ui->showPassword->text() == "Hide Password") {
// Hide: restore password mode
ui->showPassword->setText("Show Password");
ui->accountPassword->setEchoMode(QLineEdit::Password);
// Only restore dummy dots if we know a stored password exists.
// For an unsaved password, QLineEdit::Password will handle masking.
if (hasStoredPassword) {
const QSignalBlocker blocker(ui->accountPassword);
ui->accountPassword->setText(QString(8, QChar(0x2022)));
}
return;
}
// Show password
if (hasStoredPassword) {
const QString password = passCfg.getPassword();
if (!password.isEmpty()) {
const QSignalBlocker blocker(ui->accountPassword);
ui->accountPassword->setEchoMode(QLineEdit::Normal);
ui->accountPassword->setText(password);
ui->showPassword->setText("Hide Password");
}
} else if (hasFieldText) {
// No stored password, but user has typed something:
// just toggle echo mode to reveal what is currently in the field.
ui->accountPassword->setEchoMode(QLineEdit::Normal);
ui->showPassword->setText("Hide Password");
}
```
1. The snippet you provided ends in the middle of the lambda. After applying this replacement, remove any remaining original code that continues the old `showPassword` handler (e.g., the previous `else` branch, redundant `});`, or extra braces) so that there is only one complete lambda body ending with `});`.
2. Ensure that `passCfg.getPassword()` indeed returns a `QString`. If it returns another type (e.g., `std::optional<QString>`), adjust the retrieval and empty-check logic accordingly.
3. If there is centralized handling of button text (e.g., via `tr("Show Password")` / `tr("Hide Password")`), replace the string literals here with the existing translated strings to keep localization consistent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…eystroke, correctly handling paste and multi-char input
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
2 participants
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.
Make WASM auto-login behave the same as desktop: retrieve credentials immediately when GMCP negotiation completes, without requiring a click. Defer getPassword() via QTimer::singleShot(0) to avoid ASYNCIFY stack overflow from deep within the GMCP signal chain.
Improve Preferences UI: disable credential fields when "Remember my login" is unchecked, clear stored credentials on uncheck, show dummy dots for stored passwords, and delete stale IndexedDB records when the password field is cleared.
Summary by Sourcery
Implement browser-compatible credential storage and auto-login behavior for WASM, and refine login preference handling and related build configuration.
New Features:
Bug Fixes:
Enhancements:
Build: