-
Notifications
You must be signed in to change notification settings - Fork 3.6k
tui: the MCP boot chip counts expired logins separately from failures #5938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -213,15 +213,19 @@ impl SessionBootSurface { | |||||
| .filter(|row| row.state == McpServerBootState::Connecting) | ||||||
| .map(|row| row.name.as_str()) | ||||||
| .collect(); | ||||||
| // A server whose login expired is not a failure: the engine already | ||||||
| // knows the remedy (`codewhale mcp login <server>` / the login tool), | ||||||
| // so the chip counts it under the shared auth-required label and only | ||||||
| // calls the rest failed (#5926). | ||||||
| let need_login = self | ||||||
| .servers | ||||||
| .iter() | ||||||
| .filter(|row| row.state == McpServerBootState::NeedsLogin) | ||||||
| .count(); | ||||||
| let failed = self | ||||||
| .servers | ||||||
| .iter() | ||||||
| .filter(|row| { | ||||||
| matches!( | ||||||
| row.state, | ||||||
| McpServerBootState::Failed | McpServerBootState::NeedsLogin | ||||||
| ) | ||||||
| }) | ||||||
| .filter(|row| row.state == McpServerBootState::Failed) | ||||||
| .count(); | ||||||
| let connected = self | ||||||
| .servers | ||||||
|
|
@@ -238,19 +242,32 @@ impl SessionBootSurface { | |||||
| budget, | ||||||
| ); | ||||||
| } | ||||||
| if failed > 0 { | ||||||
| return activity_notice_from_candidates( | ||||||
| SessionBootActivityLevel::Failure, | ||||||
| vec![ | ||||||
| format!( | ||||||
| "MCP{ITEM_SEPARATOR}{connected} {}{ITEM_SEPARATOR}{failed} {}", | ||||||
| tr(locale, MessageId::ExtensionsStateConnected), | ||||||
| tr(locale, MessageId::PhaseFailed) | ||||||
| ), | ||||||
| format!("MCP{ITEM_SEPARATOR}{failed} failed"), | ||||||
| ], | ||||||
| budget, | ||||||
| if failed > 0 || need_login > 0 { | ||||||
| let auth_label = mcp_auth_required_state_label(); | ||||||
| let mut full = format!( | ||||||
| "MCP{ITEM_SEPARATOR}{connected} {}", | ||||||
| tr(locale, MessageId::ExtensionsStateConnected) | ||||||
| ); | ||||||
| // The narrow form drops the glyph and shortens the verb so both | ||||||
| // counts survive an 80-column footer. | ||||||
| let mut compact = String::from("MCP"); | ||||||
| if need_login > 0 { | ||||||
| full.push_str(&format!("{ITEM_SEPARATOR}{need_login} {auth_label}")); | ||||||
| compact.push_str(&format!("{ITEM_SEPARATOR}{need_login} login")); | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a needs-auth server uses a manual bearer/environment header or is plugin-contributed, the compact candidate incorrectly labels it Useful? React with 👍 / 👎. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [WARNING] Compact auth-required label is not localized The new compact fallback hard-codes the string "login" when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [INFO] Compact login-only path is not directly tested The updated single-state test uses a 100-column budget and asserts the full candidate. The new compact candidate for
Comment on lines
+254
to
+256
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [INFO] Compact chip hardcodes English 'login' The narrow candidate uses |
||||||
| } | ||||||
| if failed > 0 { | ||||||
| full.push_str(&format!( | ||||||
| "{ITEM_SEPARATOR}{failed} {}", | ||||||
| tr(locale, MessageId::PhaseFailed) | ||||||
| )); | ||||||
| compact.push_str(&format!("{ITEM_SEPARATOR}{failed} failed")); | ||||||
| } | ||||||
| let level = if failed > 0 { | ||||||
| SessionBootActivityLevel::Failure | ||||||
| } else { | ||||||
| SessionBootActivityLevel::Attention | ||||||
| }; | ||||||
| return activity_notice_from_candidates(level, vec![full, compact], budget); | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
At the supported 40-column layout, AGENTS.md reference: crates/tui/AGENTS.md:L22-L22 Useful? React with 👍 / 👎. |
||||||
| } | ||||||
| if self.phase == SessionBootPhase::Booting { | ||||||
| let count = self.servers.len().max(self.unnamed_connecting); | ||||||
|
|
@@ -644,12 +661,59 @@ mod tests { | |||||
| .map(|row| row.state), | ||||||
| Some(McpServerBootState::NeedsLogin) | ||||||
| ); | ||||||
| // The compact activity chip counts a needs-login server with the | ||||||
| // failure band (the receipt-line renderer this pinned moved to the | ||||||
| // chip in the Tideline boot-surface refactor). | ||||||
| // The compact activity chip names a needs-login server under the | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [INFO] Test comment says 'compact' but exercises full-width candidate The updated test comment says 'The compact activity chip names a needs-login server...' but the assertion calls There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test calls
Suggested change
|
||||||
| // shared auth-required label, at attention level: the remedy is a | ||||||
| // login, not a repair (#5926). | ||||||
| assert_eq!(surface.servers.len(), 1); | ||||||
| let chip = surface.activity_notice(Locale::En, 100); | ||||||
| assert!(chip.is_some(), "needs-login must surface on the boot chip"); | ||||||
| let chip = surface | ||||||
| .activity_notice(Locale::En, 100) | ||||||
| .expect("needs-login must surface on the boot chip"); | ||||||
| assert_eq!(chip.level, SessionBootActivityLevel::Attention); | ||||||
| assert_eq!( | ||||||
| chip.text, | ||||||
| format!( | ||||||
| "MCP{ITEM_SEPARATOR}0 connected{ITEM_SEPARATOR}1 {}", | ||||||
| mcp_auth_required_state_label() | ||||||
| ) | ||||||
| ); | ||||||
| assert!(!chip.text.contains("failed"), "{}", chip.text); | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [INFO] Compact needs-login-only branch lacks explicit coverage The mixed-state test covers the compact branch when both login and failure counts are present, and the first needs-login test covers the full-width branch. The compact path for |
||||||
| } | ||||||
|
|
||||||
| #[test] | ||||||
| fn chip_separates_expired_logins_from_real_failures() { | ||||||
| let mut expired = server("slack", true, false, Some("401 Unauthorized")); | ||||||
| expired.auth_required = true; | ||||||
| let snap = snapshot(vec![ | ||||||
| server("alpha", true, true, None), | ||||||
| expired, | ||||||
| server("beta", true, false, Some("Stdio transport closed")), | ||||||
| ]); | ||||||
| let surface = SessionBootSurface::from_parts( | ||||||
| Some(&snap), | ||||||
| false, | ||||||
| &[], | ||||||
| 3, | ||||||
| PluginBootSummary::default(), | ||||||
| ); | ||||||
| let chip = surface | ||||||
| .activity_notice(Locale::En, 100) | ||||||
| .expect("mixed states surface on the boot chip"); | ||||||
| assert_eq!(chip.level, SessionBootActivityLevel::Failure); | ||||||
| assert_eq!( | ||||||
| chip.text, | ||||||
| format!( | ||||||
| "MCP{ITEM_SEPARATOR}1 connected{ITEM_SEPARATOR}1 {}{ITEM_SEPARATOR}1 failed", | ||||||
| mcp_auth_required_state_label() | ||||||
| ) | ||||||
| ); | ||||||
| // Under a tight budget the compact form keeps both counts. | ||||||
| let compact = surface | ||||||
| .activity_notice(Locale::En, 30) | ||||||
| .expect("compact chip"); | ||||||
| assert_eq!( | ||||||
| compact.text, | ||||||
| format!("MCP{ITEM_SEPARATOR}1 login{ITEM_SEPARATOR}1 failed") | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| #[test] | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
app.ui_localeis non-English, this new branch emits English in both candidates:mcp_auth_required_state_label()returns◆ auth required, while the compact form hardcodeslogin; only the surrounding connected/failed labels are translated. This produces mixed-language wide and narrow footers even though localizedLaunchMcpNeedsSignInOne/LaunchMcpNeedsSignInManymessages already exist. Compose the authentication prose throughtr(locale, MessageId::...), leaving only the glyph in code.AGENTS.md reference: crates/tui/AGENTS.md:L25-L26
Useful? React with 👍 / 👎.