Control UI: keep slow reconnects out of the login gate#61345
Control UI: keep slow reconnects out of the login gate#61345coder-zhuzm wants to merge 5 commits into
Conversation
Greptile SummaryThis PR fixes a UX bug where the Control UI would incorrectly display the login gate during a slow gateway reconnect, even when the user's auth state was still valid. Previously, any
Confidence Score: 4/5Safe to merge — narrows an overly broad gate condition without touching gateway protocol, auth state, or stored credentials. Well-scoped change using structured error codes as primary signal. The redundant !showLoginGate guard is harmless. CSS specificity is correct. AUTH_RATE_LIMITED classification is debatable but pre-existing. No new risks introduced. ui/src/ui/views/overview-hints.ts — verify HIGH_CONFIDENCE_LOGIN_ERROR_PATTERNS stays aligned with normalized gateway error messages if those strings ever change in the future. Prompt To Fix All With AIThis is a comment left during a code review.
Path: ui/src/ui/app-render.ts
Line: 537-542
Comment:
**Redundant `!showLoginGate` guard in topbar pill**
Since the dashboard render path is only reached after the early-return `if (showLoginGate) { return ... }` guard on line 364, `showLoginGate` is always `false` inside the dashboard template. The `!showLoginGate` term in the condition is a no-op and can be simplified to just `!state.connected`.
```suggestion
${!state.connected
? html`<div class="pill topbar-connection-pill" title="Gateway status: Connecting">
<span class="statusDot warn" aria-hidden="true"></span>
<span>Connecting...</span>
</div>`
: nothing}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Control UI: keep slow reconnects out of ..." | Re-trigger Greptile Re-review progress:
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ${!state.connected && !showLoginGate | ||
| ? html`<div class="pill topbar-connection-pill" title="Gateway status: Connecting"> | ||
| <span class="statusDot warn" aria-hidden="true"></span> | ||
| <span>Connecting...</span> | ||
| </div>` | ||
| : nothing} |
There was a problem hiding this comment.
!showLoginGate guard in topbar pill
Since the dashboard render path is only reached after the early-return if (showLoginGate) { return ... } guard on line 364, showLoginGate is always false inside the dashboard template. The !showLoginGate term in the condition is a no-op and can be simplified to just !state.connected.
| ${!state.connected && !showLoginGate | |
| ? html`<div class="pill topbar-connection-pill" title="Gateway status: Connecting"> | |
| <span class="statusDot warn" aria-hidden="true"></span> | |
| <span>Connecting...</span> | |
| </div>` | |
| : nothing} | |
| ${!state.connected | |
| ? html`<div class="pill topbar-connection-pill" title="Gateway status: Connecting"> | |
| <span class="statusDot warn" aria-hidden="true"></span> | |
| <span>Connecting...</span> | |
| </div>` | |
| : nothing} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/ui/app-render.ts
Line: 537-542
Comment:
**Redundant `!showLoginGate` guard in topbar pill**
Since the dashboard render path is only reached after the early-return `if (showLoginGate) { return ... }` guard on line 364, `showLoginGate` is always `false` inside the dashboard template. The `!showLoginGate` term in the condition is a no-op and can be simplified to just `!state.connected`.
```suggestion
${!state.connected
? html`<div class="pill topbar-connection-pill" title="Gateway status: Connecting">
<span class="statusDot warn" aria-hidden="true"></span>
<span>Connecting...</span>
</div>`
: nothing}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Good catch. showLoginGate is already ruled out by the early return above, so this condition was redundant. I simplified it to !state.connected.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6fbb81b7d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (params.lastErrorCode) { | ||
| return LOGIN_REQUIRED_CODES.has(params.lastErrorCode); |
There was a problem hiding this comment.
shouldRenderLoginGate treats any non-null lastErrorCode as authoritative, but lastErrorCode can be a generic gateway code (for example INVALID_REQUEST) when structured details.code is absent. In that case this branch returns false and skips the high-confidence message patterns, so auth-required failures can be misclassified as reconnecting and the login gate never appears (the UI stays in a perpetual “Connecting...” shell).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. lastErrorCode can fall back to a generic gateway error code when structured auth details are absent, so treating any non-null code as authoritative was too strict. I updated the logic to return early only for known login-required codes and otherwise continue into the high-confidence message fallback.
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close as superseded: the same Control UI reconnect/login-gate problem is now tracked by the canonical open issue and replacement PR, while this branch still has a protocol-mismatch regression and no inspectable real behavior proof. Canonical path: The related issue discussion identifies #72500 and #72522 as the active canonical path; the replacement PR is open, references the canonical issue with closing syntax, and implements a sticky prior-hello state rather than this branch's string-pattern gate. So I’m closing this here and keeping the remaining discussion on #72500 and #72522. Review detailsBest possible solution: Use the canonical issue and corrected replacement PR, or an equivalent successor, to preserve the Control UI shell during transient reconnects while keeping protocol-mismatch and auth/pairing/revocation failures routed to the login gate. Do we have a high-confidence way to reproduce the issue? Yes, at source level. Current main gates every disconnected render, and this PR's predicate can be checked from source with Is this the best way to solve the issue? No. The central direction is valid, but this branch is no longer the best path because the canonical issue/replacement PR track the same root cause more directly and this branch regresses existing protocol-mismatch guidance. Security review: Security review cleared: The diff changes Control UI render gating and CSS only, with no dependency, workflow, permission, backend auth, secret-storage, or new network-call changes. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 210ff7d318db. |
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
1 participant
Summary
Connecting...status.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
!connectedstate as a reason to render the login gate, even when the user still had valid auth state and the gateway was only reconnecting.Regression Test Plan (if applicable)
User-visible / Behavior Changes
Connecting...status is shown in the topbar during reconnect states, including on mobile.Diagram (if applicable)
Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
Steps
Expected
Actual
Connecting...in the topbar.Evidence
Human Verification (required)
Connecting...status while preserving the dashboard shell.Review Conversations
Compatibility / Migration
Yes)No)No)Risks and Mitigations