Skip to content

Commit 6c6e09b

Browse files
feat(tray,frontend): use health.action as single source of truth for actions
Implements User Stories 4 and 5 from spec 013: Tray (US4 - FR-013 through FR-017): - Remove serverSupportsOAuth() URL heuristic, use health.action instead - Show Login Required for stdio OAuth servers (e.g., npx @anthropic/mcp-gcal) - Add menu items for set_secret and configure health actions - Use health.level for connected count (no legacy fallback) - Use health.detail for tooltip instead of last_error Web UI (US5 - FR-018, FR-019): - Suppress redundant last_error display when health.action conveys the issue - Add shouldShowError computed property - Login/set_secret/configure actions suppress verbose error alert Tests: - TestServerNeedsAction: verifies health.action detection - TestTrayLoginActionForStdioServers: stdio OAuth servers show login - TestConnectedCountHealthLevelOnly: no fallback to legacy connected - TestHealthActionMenuItemSelection: correct menu items per action
1 parent 1764d6a commit 6c6e09b

6 files changed

Lines changed: 490 additions & 117 deletions

File tree

frontend/src/components/ServerCard.vue

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,8 @@
5050
</div>
5151
</div>
5252

53-
<!-- Error message -->
54-
<div v-if="server.last_error" class="alert alert-error alert-sm mb-4">
53+
<!-- Error message - suppressed when health.action conveys the issue (FR-018, FR-019) -->
54+
<div v-if="shouldShowError" class="alert alert-error alert-sm mb-4">
5555
<svg class="w-4 h-4" fill="none" stroke="currentColor" viewBox="0 0 24 24">
5656
<path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M12 8v4m0 4h.01M21 12a9 9 0 11-18 0 9 9 0 0118 0z" />
5757
</svg>
@@ -269,6 +269,22 @@ const healthAction = computed(() => {
269269
return props.server.health?.action || ''
270270
})
271271
272+
// Determine if error message should be shown (FR-018, FR-019)
273+
// Suppress verbose last_error when health.action already conveys the issue
274+
const shouldShowError = computed(() => {
275+
// No error to show
276+
if (!props.server.last_error) return false
277+
278+
// Actions where the button is sufficient - error is redundant (T043-T046)
279+
const actionsSuppressingError = ['login', 'set_secret', 'configure']
280+
if (actionsSuppressingError.includes(healthAction.value)) {
281+
return false
282+
}
283+
284+
// Show error for other cases (restart, view_logs, or no action)
285+
return true
286+
})
287+
272288
const canLogout = computed(() => {
273289
// Don't show Logout button if server is disabled
274290
if (!props.server.enabled) return false

frontend/src/types/contracts.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// Generated TypeScript types from Go contracts
22
// DO NOT EDIT - This file is auto-generated by cmd/generate-types
3-
// Generated at: 2026-01-06T12:50:41-05:00
3+
// Generated at: 2026-01-06T13:08:25-05:00
44

55
export interface APIResponse<T = any> {
66
success: boolean;

internal/tray/managers.go

Lines changed: 73 additions & 106 deletions
Original file line numberDiff line numberDiff line change
@@ -344,40 +344,18 @@ func (m *MenuManager) UpdateUpstreamServersMenu(servers []map[string]interface{}
344344
}
345345

346346
// --- Update Title ---
347-
// Use health.level as source of truth for connected count (spec: Health is single source of truth)
347+
// Use health.level as single source of truth for connected count (FR-013, T039, T040)
348348
totalServers := len(servers)
349349
connectedServers := 0
350-
healthyCount := 0
351-
legacyConnectedCount := 0
352350
for _, server := range servers {
353-
serverName, _ := server["name"].(string)
354-
healthRaw := server["health"]
355-
356-
// Check health.level - "healthy" means connected
351+
// Check health.level - "healthy" means connected (no legacy fallback per FR-013)
357352
healthLevel := extractHealthLevel(server)
358353
if healthLevel == "healthy" {
359-
healthyCount++
360-
connectedServers++
361-
continue
362-
}
363-
// Fallback to legacy connected field for backward compatibility
364-
if connected, ok := server["connected"].(bool); ok && connected {
365-
legacyConnectedCount++
366354
connectedServers++
367355
}
368-
369-
// Debug: Log servers where health extraction failed but connected=true
370-
if healthLevel == "" && healthRaw != nil {
371-
m.logger.Warnw("Health extraction failed",
372-
"server", serverName,
373-
"health_type", fmt.Sprintf("%T", healthRaw),
374-
"health_raw", healthRaw)
375-
}
376356
}
377-
m.logger.Infow("Connected count calculated",
378-
"healthy", healthyCount,
379-
"legacy_connected", legacyConnectedCount,
380-
"total_connected", connectedServers,
357+
m.logger.Debugw("Connected count calculated",
358+
"healthy", connectedServers,
381359
"total_servers", totalServers)
382360
menuTitle := fmt.Sprintf("Upstream Servers (%d/%d)", connectedServers, totalServers)
383361
if m.upstreamServersMenu != nil {
@@ -657,10 +635,9 @@ func (m *MenuManager) ForceRefresh() {
657635
}
658636

659637
// getServerStatusDisplay returns display text, tooltip, and icon data for a server
660-
// Uses the unified health status from the backend when available
638+
// Uses the unified health status from the backend as single source of truth (FR-013, FR-016, FR-017)
661639
func (m *MenuManager) getServerStatusDisplay(server map[string]interface{}) (displayText, tooltip string, iconData []byte) {
662640
serverName, _ := server["name"].(string)
663-
lastError, _ := server["last_error"].(string)
664641
shouldRetry, _ := server["should_retry"].(bool)
665642

666643
var retryCount int
@@ -782,17 +759,13 @@ func (m *MenuManager) getServerStatusDisplay(server map[string]interface{}) (dis
782759
var tooltipLines []string
783760
tooltipLines = append(tooltipLines, fmt.Sprintf("%s - %s", serverName, statusText))
784761

785-
// Add health detail if available
762+
// Use health.detail for tooltip details instead of last_error (FR-017, T034)
786763
if hasHealth {
787764
if detail, ok := healthData["detail"].(string); ok && detail != "" {
788765
tooltipLines = append(tooltipLines, fmt.Sprintf("Detail: %s", detail))
789766
}
790767
}
791768

792-
if lastError != "" {
793-
tooltipLines = append(tooltipLines, fmt.Sprintf("Last error: %s", lastError))
794-
}
795-
796769
if shouldRetry {
797770
if retryCount > 0 {
798771
tooltipLines = append(tooltipLines, fmt.Sprintf("Retry scheduled (attempts: %d)", retryCount))
@@ -812,49 +785,19 @@ func (m *MenuManager) getServerStatusDisplay(server map[string]interface{}) (dis
812785
return
813786
}
814787

815-
// serverSupportsOAuth determines if a server supports OAuth authentication
816-
func (m *MenuManager) serverSupportsOAuth(server map[string]interface{}) bool {
817-
// Get server URL
818-
serverURL, ok := server["url"].(string)
819-
if !ok || serverURL == "" {
820-
return false
821-
}
822-
823-
// Check if it's an HTTP/HTTPS server (OAuth is typically used with HTTP-based APIs)
824-
urlLower := strings.ToLower(serverURL)
825-
if !strings.HasPrefix(urlLower, "http://") && !strings.HasPrefix(urlLower, "https://") {
788+
// serverNeedsAction checks if a server needs a specific action based on health.action
789+
// This replaces the old URL-based serverSupportsOAuth heuristic (FR-014)
790+
func (m *MenuManager) serverNeedsAction(server map[string]interface{}, action string) bool {
791+
healthData, ok := server["health"].(map[string]interface{})
792+
if !ok {
826793
return false
827794
}
828-
829-
// Check for OAuth-related URLs patterns
830-
if strings.Contains(urlLower, "oauth") || strings.Contains(urlLower, "auth") {
831-
return true
832-
}
833-
834-
// For common MCP servers that we know support OAuth
835-
oauthDomains := []string{
836-
"sentry.dev",
837-
"github.com",
838-
"gitlab.com",
839-
"google.com",
840-
"googleapis.com",
841-
"microsoft.com",
842-
"oauth.com",
843-
}
844-
845-
for _, domain := range oauthDomains {
846-
if strings.Contains(urlLower, domain) {
847-
return true
848-
}
849-
}
850-
851-
// For any HTTP/HTTPS server, show OAuth option since it might support it
852-
// Users can try it and it will fail gracefully if not supported
853-
return true
795+
healthAction, _ := healthData["action"].(string)
796+
return healthAction == action
854797
}
855798

856-
// createServerActionSubmenus creates action submenus for a server (enable/disable, quarantine, OAuth login, restart)
857-
// Uses health.action to determine which actions are most relevant
799+
// createServerActionSubmenus creates action submenus for a server based on health.action
800+
// Uses health.action as single source of truth for determining which actions to show (FR-014, FR-015)
858801
func (m *MenuManager) createServerActionSubmenus(serverMenuItem *systray.MenuItem, server map[string]interface{}) {
859802
serverName, _ := server["name"].(string)
860803
if serverName == "" {
@@ -864,13 +807,13 @@ func (m *MenuManager) createServerActionSubmenus(serverMenuItem *systray.MenuIte
864807
enabled, _ := server["enabled"].(bool)
865808
quarantined, _ := server["quarantined"].(bool)
866809

867-
// Get health.action if available
810+
// Get health.action - this is the single source of truth for what action is needed
868811
healthAction := ""
869812
if healthData, ok := server["health"].(map[string]interface{}); ok {
870813
healthAction, _ = healthData["action"].(string)
871814
}
872815

873-
// Enable/Disable action
816+
// Enable/Disable action - always shown
874817
var enableText string
875818
if enabled {
876819
enableText = textDisable
@@ -880,40 +823,67 @@ func (m *MenuManager) createServerActionSubmenus(serverMenuItem *systray.MenuIte
880823
enableItem := serverMenuItem.AddSubMenuItem(enableText, fmt.Sprintf("%s server %s", enableText, serverName))
881824
m.serverActionItems[serverName] = enableItem
882825

883-
// Restart action (for stdio servers when health.action is "restart" or server has errors)
884-
if enabled && !quarantined {
885-
restartItem := serverMenuItem.AddSubMenuItem("🔄 Restart", fmt.Sprintf("Restart server %s", serverName))
886-
m.serverRestartItems[serverName] = restartItem
826+
// Show action-specific menu items based on health.action (FR-014, FR-015, FR-036-038)
827+
if !quarantined && enabled {
828+
switch healthAction {
829+
case "login":
830+
// Login Required - show prominently (FR-015, T036)
831+
oauthItem := serverMenuItem.AddSubMenuItem("⚠️ Login Required", fmt.Sprintf("Authenticate with %s using OAuth", serverName))
832+
m.serverOAuthItems[serverName] = oauthItem
833+
go func(name string, item *systray.MenuItem) {
834+
for range item.ClickedCh {
835+
if m.onServerAction != nil {
836+
go m.onServerAction(name, "oauth_login")
837+
}
838+
}
839+
}(serverName, oauthItem)
887840

888-
// Set up restart click handler
889-
go func(name string, item *systray.MenuItem) {
890-
for range item.ClickedCh {
891-
if m.onServerAction != nil {
892-
go m.onServerAction(name, "restart")
841+
case "set_secret":
842+
// Set Secret - opens Web UI secrets page (T037)
843+
secretItem := serverMenuItem.AddSubMenuItem("⚠️ Set Secret", fmt.Sprintf("Configure missing secret for %s", serverName))
844+
go func(name string, item *systray.MenuItem) {
845+
for range item.ClickedCh {
846+
if m.onServerAction != nil {
847+
go m.onServerAction(name, "set_secret")
848+
}
893849
}
894-
}
895-
}(serverName, restartItem)
896-
}
850+
}(serverName, secretItem)
897851

898-
// OAuth Login action (only for servers that support OAuth)
899-
if m.serverSupportsOAuth(server) && !quarantined {
900-
// Highlight login if health.action suggests it
901-
loginLabel := "🔐 OAuth Login"
902-
if healthAction == "login" {
903-
loginLabel = "⚠️ Login Required"
904-
}
905-
oauthItem := serverMenuItem.AddSubMenuItem(loginLabel, fmt.Sprintf("Authenticate with %s using OAuth", serverName))
906-
m.serverOAuthItems[serverName] = oauthItem
852+
case "configure":
853+
// Configure - opens Web UI server config (T038)
854+
configItem := serverMenuItem.AddSubMenuItem("⚠️ Configure", fmt.Sprintf("Fix configuration for %s", serverName))
855+
go func(name string, item *systray.MenuItem) {
856+
for range item.ClickedCh {
857+
if m.onServerAction != nil {
858+
go m.onServerAction(name, "configure")
859+
}
860+
}
861+
}(serverName, configItem)
907862

908-
// Set up OAuth login click handler
909-
go func(name string, item *systray.MenuItem) {
910-
for range item.ClickedCh {
911-
if m.onServerAction != nil {
912-
// Run in new goroutines to avoid blocking the event channel
913-
go m.onServerAction(name, "oauth_login")
863+
case "restart":
864+
// Restart suggested by health - show prominently
865+
restartItem := serverMenuItem.AddSubMenuItem("⚠️ Restart Required", fmt.Sprintf("Restart server %s to fix issues", serverName))
866+
m.serverRestartItems[serverName] = restartItem
867+
go func(name string, item *systray.MenuItem) {
868+
for range item.ClickedCh {
869+
if m.onServerAction != nil {
870+
go m.onServerAction(name, "restart")
871+
}
914872
}
915-
}
916-
}(serverName, oauthItem)
873+
}(serverName, restartItem)
874+
875+
default:
876+
// No specific action needed - show standard restart option
877+
restartItem := serverMenuItem.AddSubMenuItem("🔄 Restart", fmt.Sprintf("Restart server %s", serverName))
878+
m.serverRestartItems[serverName] = restartItem
879+
go func(name string, item *systray.MenuItem) {
880+
for range item.ClickedCh {
881+
if m.onServerAction != nil {
882+
go m.onServerAction(name, "restart")
883+
}
884+
}
885+
}(serverName, restartItem)
886+
}
917887
}
918888

919889
// Quarantine action (only if not already quarantined)
@@ -925,7 +895,6 @@ func (m *MenuManager) createServerActionSubmenus(serverMenuItem *systray.MenuIte
925895
go func(name string, item *systray.MenuItem) {
926896
for range item.ClickedCh {
927897
if m.onServerAction != nil {
928-
// Run in new goroutines to avoid blocking the event channel
929898
go m.onServerAction(name, "quarantine")
930899
}
931900
}
@@ -936,8 +905,6 @@ func (m *MenuManager) createServerActionSubmenus(serverMenuItem *systray.MenuIte
936905
go func(name string, item *systray.MenuItem) {
937906
for range item.ClickedCh {
938907
if m.onServerAction != nil {
939-
// The best approach is to have the sync manager handle the toggle.
940-
// We send a generic 'toggle_enable' action and let the handler determine the state.
941908
go m.onServerAction(name, "toggle_enable")
942909
}
943910
}

0 commit comments

Comments
 (0)