feat(macos): calm "Sign in" affordance for OAuth login-required servers (MCP-1822)#629
Conversation
…rs (MCP-1822)
A server in the OAuth login-required state (health.action == "login")
previously read as a hard error in the tray: pre-T1 the backend classifies
it as an error-severity MCPX_UNKNOWN_UNCLASSIFIED diagnostic, so the same
server surfaced in the red "Fix issues" section (xmark.octagon "file a bug")
and tinted the tray icon badge red — while the login affordance was a plain
"Log In" menu item.
Render it as a calm, actionable Sign-in state instead, keyed off the stable
cross-surface `health.action == "login"` contract (forward-compatible with
T1's classifier change):
- ServerStatus.isOAuthLoginRequired helper.
- AppState.serversWithDiagnostic / worstDiagnosticSeverity exclude
login-required servers, so they no longer appear in the red "Fix issues"
group or tint the tray badge. They remain in the calm "Needs Attention"
group, which now shows the remediation verb ("Sign in") explicitly.
- Rename the affordance "Log In" -> "Sign in" across the tray (HealthAction
label, server submenu, ServerDetail header button, server-list context
menu), mirroring the Web UI's calm tone.
Tests: AppStateOAuthSignInTests covers exclusion from Fix-issues / badge,
retention in Needs-Attention, and that genuine non-OAuth errors still
surface; ModelsTests asserts the new "Sign in" label.
Related #MCP-1819
Deploying mcpproxy-docs with
|
| Latest commit: |
b58ffe4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7232cf76.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://mcp-1822-tray-signin-afforda.mcpproxy-docs.pages.dev |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 27252179857 --repo smart-mcp-proxy/mcpproxy-go
|
Dumbris
left a comment
Dumbris
left a comment
There was a problem hiding this comment.
Code Review: MCP-1834 — macOS tray Sign-in affordance
Overall: Core logic is sound and test coverage is thorough. One structural issue before merge: isOAuthLoginRequired was added as the "stable, cross-surface contract" but most call sites still use the raw check.
Required fix
Inconsistent use of isOAuthLoginRequired
AppState.swift correctly adopts the new computed property. But three call sites still use the raw health?.action == "login" check:
TrayMenu.swift:256ServerDetailView.swift:144ServersView.swift:507
Change all three to server.isOAuthLoginRequired so the abstraction is actually used and a future contract change only needs to happen in one place.
Observation (non-blocking)
The new label block in TrayMenu.swift shows the action label for any health action, not only login. A restart-needed server would also show "Restart" as secondary text. Confirm this is intentional or add an isOAuthLoginRequired guard.
What's good
isOAuthLoginRequiredcomputed property — correct abstraction- AppState exclusion logic (Fix Issues + badge tint) — correct and well-commented
AppStateOAuthSignInTests.swift— comprehensive, covers all three AppState facets- Consistent "Sign in" rename across all surfaces
Fix the three raw health?.action == "login" checks → this is ready to merge.
…ope attention verb to login (MCP-1822 review) Address review on PR #629 (MCP-1834): - Replace the three raw `health?.action == "login"` checks (TrayMenu submenu, ServerDetail header button, ServersView context menu) with the `ServerStatus.isOAuthLoginRequired` helper, so the stable contract has a single definition and call site. - Guard the Needs-Attention trailing remediation label to the login state only (this issue's scope); other attention actions keep their prior icon-only row. Behavior-identical for the login path; AppStateOAuthSignInTests + label test remain green.
|
Thanks for the review (MCP-1834) — both points addressed in Required fix (done): the three call sites now use Non-blocking observation (guarded): the Needs-Attention trailing remediation label is now scoped to the login state only (
|
The calm 'Sign in' affordance from PR #629 was only applied to the SwiftUI TrayMenu (MenuBarExtra), but the actually-active macOS tray is the AppKit AppController in MCPProxyApp.swift. There the OAuth login-required server still rendered the old alarming treatment: a red lock-badge overlay and a 'Log In (Opens Browser)' button. Bring the active AppKit tray in line with the cross-surface contract: - Use server.isOAuthLoginRequired (Models.swift contract) instead of the raw health?.action == "login" comparison for the menu's needsAuth check. - Add ServerStatus.menuStatusNSColor: the login-required state gets the calm system accent tint, not the red statusNSColor error dot; drop the red lock-badge overlay. Genuine (non-login) errors keep statusNSColor. - Rename the menu action to the calm 'Sign in' verb (button + Needs Attention label), matching the SwiftUI affordance. - Add unit coverage for the shared menuStatusNSColor logic mirroring AppStateOAuthSignInTests. Related #629 Related MCP-1856
![mcpproxy-gatekeeper[bot]](https://avatars.githubusercontent.com/u/216428285?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Gatekeeper approval — Codex review verdict: ACCEPT.
This approval is posted automatically by the MCPProxy Gatekeeper App on behalf of the Codex reviewer (verdict of record lives in the Paperclip review thread). Author≠approver satisfied; QA + CI gates enforced separately.
Auto-approved per Model B (MCP-1249).
There was a problem hiding this comment.
✅ Gatekeeper approval — Codex review verdict: ACCEPT.
This approval is posted automatically by the MCPProxy Gatekeeper App on behalf of the Codex reviewer (verdict of record lives in the Paperclip review thread). Author≠approver satisfied; QA + CI gates enforced separately.
Auto-approved per Model B (MCP-1249).
…P-2084) (#634) Some remote MCP endpoints (e.g. Google's sqladmin MCP) allow anonymous `initialize` + `tools/list` but enforce OAuth only at `tools/call`. The proxy connected, listed 15 tools, and the health classifier reported the server as fully healthy/Ready with no Sign-in affordance — every actual tool call failed at runtime with "authorization required", which the operator only discovered by failing a call. Root cause: the health calculator's OAuth/login branch is gated entirely on `OAuthRequired`, which is derived from connect-time OAuth config only. A server that connects anonymously has `OAuthRequired=false`, and a call-time "authorization required" is not a connection error, so the state machine stays Ready and nothing flags the server for sign-in. Fix (backend detection; reuses the existing action=login Sign-in CTA): - managed client: on a tools/call that fails with "authorization required" / 401 from an otherwise-connected server, set a per-server oauthCallRequired flag (cleared by a successful call or a fresh post-sign-in Connect). - runtime: feed the flag into the health calculator input as CallTimeOAuthRequired. - health calculator: a connected server with CallTimeOAuthRequired now returns Level=degraded, Action=login, Summary="Sign-in required" — placed after the connection-state switch so genuine error/disconnected states keep priority. This closes the gap MCP-1819 left (which only handled servers surfacing login-required at connect time). No new config keys, CLI flags, or API endpoints; the documented health contract (action=login) is unchanged. Tests: health calculator branch (+ negative: connection errors keep priority), managed-client call-time signal classification (auth vs 401 vs non-auth vs connection error). Related MCP-1819 (#628/#629/#630).
2 participants
What & why
Parent epic: smart-mcp-proxy/mcpproxy-go MCP-1819 — OAuth login-required = actionable Sign-in state, not MCPX_UNKNOWN_UNCLASSIFIED. This is T3 (macOS tray).
A server in the OAuth login-required state (
health.action == "login") read as a hard error in the tray. Pre-T1 the backend classifies that state as anerror-severityMCPX_UNKNOWN_UNCLASSIFIEDdiagnostic, so the same server appeared in the red "Fix issues" section (xmark.octagon/ "file a bug") and tinted the tray-icon badge red — while the only login affordance was a plain "Log In" menu item.This renders it as a calm, actionable Sign-in state instead.
Changes (all in
native/macos/)ServerStatus.isOAuthLoginRequired— keyed off the stable, cross-surfacehealth.action == "login"contract (the same one CLI/REST/Web-UI/TUI use).AppState.serversWithDiagnosticandworstDiagnosticSeverityexclude login-required servers → no red "Fix issues" entry, no red badge tint for a pure sign-in state. They remain in the calm "Needs Attention" group, which now shows the remediation verb ("Sign in") explicitly so the affordance isn't buried.HealthAction.login.label, server submenu, ServerDetail header button, server-list context menu), mirroring the Web UI's calm tone.Contract / T1 relationship
Keyed off the already-stable
action == "login"contract that exists onmain, so this is independently correct and forward-compatible with T1's classifier change (whether T1 stops emitting a diagnostic for OAuth-pending or emits a calmer one, the tray no longer error-frames the sign-in state).Tests
swift test— newAppStateOAuthSignInTests(6) + updatedModelsTests.testHealthActionLabelsall pass:HealthAction.login.label == "Sign in"Pre-existing, environment-dependent failures unrelated to this change (
AutoStartTests.testFirstRunFlagRoundTrip— UserDefaults pollution;MergePatchEncodingTestsandSSEParserTests— Foundation-version canaries) reproduce on cleanorigin/mainand are not touched here.Verification note
A live OAuth-pending upstream cannot be staged in this environment, so visual
mcpproxy-ui-testcapture of the exact login state isn't meaningful here; behavior is covered by the unit tests above against theAppState/ServerStatuslogic that drives the menu.Related #MCP-1819