fix: improve profile credential display for oauth and env refs#86
fix: improve profile credential display for oauth and env refs#86
Conversation
📊 Test Coverage Report
Coverage measured by |
dev01lay2
left a comment
There was a problem hiding this comment.
PR #86 — fix: improve profile credential display for oauth and env refs
CI still pending at review time.
Design is solid overall
Credential-aware rendering ( / / / ), the helper with backend-first kind resolution + frontend inference fallback, the dual-engine Doctor refactor, Doctor chat history persistence, and the OAuth flow in the profile dialog are all well-structured. Test coverage is thorough.
BS — restoreFromCache marks zeroclaw sessions as live-connected
restoreFromCache unconditionally calls:
setConnected(true);
setBridgeConnected(nextEngine === "zeroclaw");For the openclaw engine (stateless HTTP) connected: true is fine.
For the zeroclaw engine (event-driven Tauri bridge), there is no active bridge after a page transition — the old session emitted no new events and may be dead on the backend. Setting both connected and bridgeConnected to true surfaces the "Connected" + "Bridge Connected" badges to the user and suppresses the "Reconnect" path. If the backend session has expired, the user's next message gets swallowed silently. Fix: do not set connected: true for zeroclaw cache restores; leave it false so the disconnected-with-history branch renders and the user can explicitly reconnect:
setConnected(nextEngine === "openclaw");
setBridgeConnected(false);NBS 1 — is_oauth_provider_alias includes plain "openai"
fn is_oauth_provider_alias(provider: &str) -> bool {
matches!(..., "openai-codex" | "openai_codex" | "github-copilot" | "copilot" | "openai")
}"openai" is a standard API-key provider. Any provider=openai profile whose auth_ref happens to start with openai: (e.g. a copy-paste of a codex ref) will be classified as credentialKind = OAuth by infer_resolved_credential_kind. This bleeds into the Settings credential display. Meanwhile providerUsesOAuthAuth in the frontend (Settings.tsx) only returns true for the normalized "openai-codex" string, so the credential source dropdown won't offer OAuth for plain openai — the two sides disagree. Suggest dropping "openai" from the backend alias set.
NBS 2 — Chat.tsx stale agentId in the instance-change effect
useEffect(() => {
setMessages([]);
setAgentId("");
...
ua.listAgents().then((list) => {
const nextAgent = ids.includes(agentId) && agentId ? agentId : (ids[0] || "");
...
});
}, [ua.instanceId, ua]); // agentId not in depsagentId is read inside the then callback but is not a dependency, so when ua.instanceId changes it captures the stale value from the previous instance. The immediately preceding setAgentId("") resets it in state but the closure has already closed over the old ref. Add agentId to the dependency array (or read the latest value via a ref).
dev01lay2
left a comment
There was a problem hiding this comment.
PR #86 review
CI still pending at review time.
Design is solid overall — credential-aware rendering, the profile-credential.ts helper with backend-first kind resolution + frontend inference fallback, the dual-engine Doctor refactor, Doctor chat history persistence, and the in-dialog OAuth flow are all well-structured. Test coverage is thorough.
BS — restoreFromCache marks zeroclaw sessions as live-connected
restoreFromCache unconditionally calls:
setConnected(true);
setBridgeConnected(nextEngine === "zeroclaw");
For openclaw (stateless HTTP) connected=true is fine. For zeroclaw (event-driven Tauri bridge), there is no active bridge after a page transition — the session may be dead on the backend. Setting both connected and bridgeConnected to true surfaces "Connected" + "Bridge Connected" badges and suppresses the "Reconnect" path. If the backend session has expired, the user's next message gets swallowed silently.
Fix: set connected=false for zeroclaw cache restores so the disconnected-with-history branch renders and the user can explicitly reconnect:
setConnected(nextEngine === "openclaw");
setBridgeConnected(false);
NBS 1 — is_oauth_provider_alias includes plain "openai"
fn is_oauth_provider_alias(provider: &str) -> bool {
matches!(..., "openai-codex" | "openai_codex" | "github-copilot" | "copilot" | "openai")
}
"openai" is a standard API-key provider. Any provider=openai profile whose auth_ref starts with "openai:" gets classified as credentialKind=OAuth by infer_resolved_credential_kind. Meanwhile providerUsesOAuthAuth in Settings.tsx only returns true for normalized "openai-codex" — the two sides disagree. Suggest dropping "openai" from the backend alias set.
NBS 2 — Chat.tsx stale agentId in the instance-change effect
useEffect(() => {
setMessages([]);
setAgentId("");
...
ua.listAgents().then((list) => {
const nextAgent = ids.includes(agentId) && agentId ? agentId : (ids[0] || "");
...
});
}, [ua.instanceId, ua]); // agentId not in deps
agentId is read inside the then callback but not listed as a dependency, so when ua.instanceId changes the closure captures the stale pre-reset value. Add agentId to the dep array or read via a ref.
📦 PR Build Artifacts
|
dev01lay2
left a comment
There was a problem hiding this comment.
PR #86 review (re-review, commits 3bce673 + b90d4fe)
CI: rust / frontend / coverage / profile-e2e all pass. Build matrix still pending (pre-existing infra).
All three blocking/non-blocking issues from the prior round are addressed.
✅ BS resolved — restoreFromCache zeroclaw false-connected
setConnected(nextEngine === "openclaw") + setBridgeConnected(false) is exactly right. Zeroclaw cache restores now land in the disconnected-with-history branch; users can explicitly reconnect rather than silently losing messages to a dead session.
✅ NBS 1 resolved — is_oauth_provider_alias / OAUTH_PROVIDER_ALIASES
"openai" dropped from both the Rust alias function and the frontend OAUTH_PROVIDER_ALIASES set. New backend test infer_resolved_credential_kind_does_not_treat_plain_openai_as_oauth pins the contract. Backend and frontend are now in agreement.
✅ NBS 2 resolved — Chat.tsx stale agentId closure
agentIdRef pattern is the right call here. The sync effect keeps the ref current on every render; the instance-change effect reads agentIdRef.current as previousAgentId before the reset — correctly capturing the pre-transition value without adding agentId to the dep array (which would re-trigger the effect on every agent switch).
NBS — env_ref credential rows don't show resolution status
showCredentialStatus = credential.kind !== "env_ref" hides the status line for env-ref profiles. The user can see the env var name via credentialRef, but there's no visual indication of whether the variable is actually populated in the environment. An env var that's misconfigured or missing looks identical to one that resolves correctly. Consider rendering the status for env_ref as well (or rendering both ref and status), especially since the backend does compute and return resolved for env-ref entries.
Summary
API Keyprofile-row display with credential-aware rendering (kind/ref/status)resolve_api_keys/remote_resolve_api_keyspayload withcredentialKind,authRef, andresolvedopenai-codex:*can show session-ready state instead of misleadingnot setDetails
oauth,env_ref,manual, orunsetValidation
bun test(pass)cargo test infer_resolved_credential_kind(pass)