fix(settings): 避免凭据加载前被空值覆盖#82
Merged
Merged
Conversation
Settings credential inputs can mount empty while readCredential is still pending. The field now waits for a successful read before enabling saves, tracks whether the user actually edited the value, and surfaces read failures without writing an empty credential back to storage. Constraint: Issue Open-Less#34 requires preserving existing credentials when the settings page is opened or blurred without edits. Constraint: Keep secrets out of logs and UI. Rejected: Add full E2E coverage now | larger test harness work than needed for the minimal bug fix. Confidence: high Scope-risk: narrow Tested: npm install Tested: npm run build Tested: cargo check Tested: git diff --check
Reviewer's GuideRefines the Settings credential input lifecycle to distinguish loading/ready/error states, gate user actions until credentials are loaded, and ensure only user-initiated edits trigger saves, preventing empty-value overwrites while improving status feedback and i18n coverage. Sequence diagram for credential loading and guarded savesequenceDiagram
actor User
participant SettingsPage
participant CredentialField
participant CredentialStore
User->>SettingsPage: openSettings()
SettingsPage->>CredentialField: mountCredentialField(account)
activate CredentialField
CredentialField->>CredentialField: useEffect_loadCredential()
Note over CredentialField: Initialize
CredentialField->>CredentialField: setLoaded(false)
CredentialField->>CredentialField: setDirty(false)
CredentialField->>CredentialField: setStatus(idle)
CredentialField->>CredentialField: setValue("")
CredentialField->>CredentialStore: readCredential(account)
alt read success
CredentialStore-->>CredentialField: credentialValue
CredentialField->>CredentialField: setValue(credentialValue)
CredentialField->>CredentialField: setLoaded(true)
else read failure
CredentialStore-->>CredentialField: error
CredentialField->>CredentialField: console.error(...)
CredentialField->>CredentialField: setLoaded(false)
CredentialField->>CredentialField: setStatus(readError)
end
User->>CredentialField: typeInInput()
CredentialField->>CredentialField: handleChange(event)
CredentialField->>CredentialField: setValue(newValue)
alt loaded is false
Note over CredentialField: return early, dirty stays false
else loaded is true
CredentialField->>CredentialField: setDirty(true)
CredentialField->>CredentialField: clearTimeout(debounceRef)
CredentialField->>CredentialField: debounceRef = setTimeout(save(newValue), 300ms)
end
User->>CredentialField: blurInput()
CredentialField->>CredentialField: onBlur()
Note over CredentialField: save() guarded by loaded && dirty
CredentialField->>CredentialField: save(value)
alt !loaded or !dirty
Note over CredentialField: save() returns immediately
else loaded && dirty
CredentialField->>CredentialField: setStatus(saving)
CredentialField->>CredentialStore: setCredential(account, value)
alt save success
CredentialStore-->>CredentialField: ok
CredentialField->>CredentialField: setDirty(false)
CredentialField->>CredentialField: showTemporaryStatus(saved)
else save failure
CredentialStore-->>CredentialField: error
CredentialField->>CredentialField: console.error(...)
CredentialField->>CredentialField: showTemporaryStatus(saveError)
end
end
deactivate CredentialField
Class diagram for updated CredentialField state and behaviorclassDiagram
class CredentialField {
- string label
- string account
- string placeholder
- boolean mono
- boolean mask
- string defaultValue
- string value
- boolean revealed
- boolean loaded
- boolean dirty
- Status status
- TimeoutHandle debounceRef
- TimeoutHandle statusRef
+ useEffect_loadCredential()
+ useEffect_cleanup()
+ showTemporaryStatus(nextStatus)
+ save(v)
+ handleChange(event)
+ onBlur()
+ fillDefault()
+ onCopy()
}
class Status {
<<enumeration>>
idle
saving
saved
readError
saveError
copied
copyError
}
class TimeoutHandle {
<<type>>
}
CredentialField --> Status : uses
CredentialField --> TimeoutHandle : uses
%% Behavioral constraints
class SaveConstraints {
+ boolean loaded
+ boolean dirty
}
CredentialField --> SaveConstraints : guards save
%% Notes: save() only runs when loaded && dirty
State diagram for CredentialField loading and status lifecyclestateDiagram-v2
[*] --> Loading
state Loading {
[*] --> WaitingRead
WaitingRead --> Ready : readSuccess
WaitingRead --> ReadError : readFailure
}
Loading --> Ready : setLoaded(true)
ReadError --> [*]
Ready --> Saving : userEdits && blurOrDebounce
Saving --> Saved : saveSuccess
Saving --> SaveError : saveFailure
Saved --> Ready : temporaryStatusTimeout
SaveError --> Ready : temporaryStatusTimeout
Ready --> Copied : copySuccess
Ready --> CopyError : copyFailure
Copied --> Ready : temporaryStatusTimeout
CopyError --> Ready : temporaryStatusTimeout
state Ready {
[*] --> Idle
Idle --> Saving : dirty && saveRequested
Idle --> Copied : copySuccess
Idle --> CopyError : copyFailure
}
state Saved {
[*] --> DisplaySaved
DisplaySaved --> Idle : temporaryStatusTimeout
}
state SaveError {
[*] --> DisplaySaveError
DisplaySaveError --> Idle : temporaryStatusTimeout
}
state Copied {
[*] --> DisplayCopied
DisplayCopied --> Idle : temporaryStatusTimeout
}
state CopyError {
[*] --> DisplayCopyError
DisplayCopyError --> Idle : temporaryStatusTimeout
}
File-Level Changes
Assessment against linked issues
Possibly linked issues
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 1 issue, and left some high level feedback:
- When
readCredentialfails you setloadedback tofalseand use a temporaryreadErrorstatus that resets toidle, which leaves the field permanently disabled with a “loading” placeholder and no visible error after 1.6s; consider making the error state persistent and allowing some kind of recovery (e.g., settingloadedtotruein the error path or introducing an expliciterror/readyflag instead of overloadingloaded). - The
showTemporaryStatushelper types itsnextargument astypeof status, which is the runtime value type (string) rather than the union you defined inuseState; it would be safer to extract atype Status = 'idle' | 'saving' | 'saved' | 'readError' | 'saveError' | 'copied' | 'copyError'and use that consistently for state and helper signatures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When `readCredential` fails you set `loaded` back to `false` and use a temporary `readError` status that resets to `idle`, which leaves the field permanently disabled with a “loading” placeholder and no visible error after 1.6s; consider making the error state persistent and allowing some kind of recovery (e.g., setting `loaded` to `true` in the error path or introducing an explicit `error`/`ready` flag instead of overloading `loaded`).
- The `showTemporaryStatus` helper types its `next` argument as `typeof status`, which is the runtime value type (`string`) rather than the union you defined in `useState`; it would be safer to extract a `type Status = 'idle' | 'saving' | 'saved' | 'readError' | 'saveError' | 'copied' | 'copyError'` and use that consistently for state and helper signatures.
## Individual Comments
### Comment 1
<location path="openless-all/app/src/pages/Settings.tsx" line_range="374-378" />
<code_context>
.catch(error => {
if (cancelled) return;
console.error('[settings] failed to read credential', account, error);
- setLoaded(true);
- setStatus('saveError');
+ setLoaded(false);
+ setStatus('readError');
});
return () => {
</code_context>
<issue_to_address>
**issue (bug_risk):** Leaving `loaded` as `false` on read error keeps the field permanently disabled.
With `setLoaded(false)` in the read failure branch and `disabled = !loaded`, a read error now leaves the input and buttons permanently disabled, so users cannot fix or enter credentials after a failed fetch. Previously, setting `loaded` to `true` after a read error allowed editing despite the failure. Consider keeping `loaded` as `true` on read errors so the field remains interactive while `readError` is shown.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Review feedback identified that read failures left fields disabled while the temporary status disappeared. Read errors now leave the field interactive with a persistent readError status, and the credential-field status union is named explicitly for safer helper signatures. Constraint: Preserve the issue Open-Less#34 guard that only dirty loaded fields save credentials. Rejected: Broader state-machine rewrite | unnecessary for this targeted review fix. Confidence: high Scope-risk: narrow Tested: npm run build Tested: cargo check Tested: git diff --check
appergb
pushed a commit
that referenced
this pull request
Apr 30, 2026
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.
摘要
Fixes #34。
本 PR 按最小改动修复设置页凭据字段在加载完成前失焦时，可能把已有密钥清空的问题。
此前
CredentialField在readCredential()异步读取完成前，字段初始值可能仍为空。如果此时发生 blur，旧逻辑会保存当前空值，导致已有 ASR / LLM 凭据被覆盖或移除。本次改动为凭据字段增加明确的 loading / ready / error 状态，并用 dirty 标记区分“用户实际修改”和“只是字段初始化为空”，避免打开设置页但未修改字段时写回空值。
修复 / 新增 / 改进
CredentialField增加状态管理：loadingreadyerrorreadCredential()完成前禁用以下操作：使用
dirtyRef控制保存时机：readCredential()失败时显示“读取失败”。readCredential()失败时不调用setCredential()，避免用空值覆盖已有凭据。不在日志或 UI 中打印密钥明文。
兼容
不包含：
对现有用户 / 本地环境 / 构建流程的影响：
测试计划
命令：
npm run build结果：通过
证据路径：本地构建输出
命令：
cargo check结果：通过，仅有既有 warning
证据路径：本地检查输出
命令：
git diff --check结果：通过
证据路径：本地命令输出
主要改动文件
openless-all/app/src/pages/Settings.tsx备注
本 PR 仅修复凭据字段生命周期导致的空值覆盖问题，不扩大到 provider 配置、凭据存储格式或后端读取逻辑调整。
Summary by Sourcery
Prevent credential fields in the settings page from overwriting existing secrets with empty values before asynchronous loading completes, and surface clearer status feedback to users.
Bug Fixes:
Enhancements: