From b37649e45299d426aeae73a54efc0d1155fb5c91 Mon Sep 17 00:00:00 2001 From: huaiyuWangh <34158348+huaiyuWangh@users.noreply.github.com> Date: Fri, 17 Apr 2026 04:40:02 +0800 Subject: [PATCH] feat(ui): add skills discovery status to sidebar and landing page (#2384) - Display loaded/errored skills in sidebar and landing page - Use pubsub event to carry skill states directly to UI, avoiding global mutable state - Subscribe to skills discovery events via broker, consistent with MCP/LSP patterns - Rename State to DiscoveryState for clarity --- internal/app/app.go | 2 + internal/skills/skills.go | 48 ++++++++++++ internal/skills/skills_test.go | 58 +++++++++++++- internal/ui/model/landing.go | 5 +- internal/ui/model/sidebar.go | 114 ++++++++++++++++++--------- internal/ui/model/skills.go | 127 +++++++++++++++++++++++++++++++ internal/ui/model/skills_test.go | 58 ++++++++++++++ internal/ui/model/ui.go | 13 +++- 8 files changed, 383 insertions(+), 42 deletions(-) create mode 100644 internal/ui/model/skills.go create mode 100644 internal/ui/model/skills_test.go diff --git a/internal/app/app.go b/internal/app/app.go index 987d0ec06022a45c6c472046db652d1d27241cc3..f6de3f7a083d26cfd09aa35b70718e9db9be004d 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -34,6 +34,7 @@ import ( "github.com/charmbracelet/crush/internal/pubsub" "github.com/charmbracelet/crush/internal/session" "github.com/charmbracelet/crush/internal/shell" + "github.com/charmbracelet/crush/internal/skills" "github.com/charmbracelet/crush/internal/ui/anim" "github.com/charmbracelet/crush/internal/ui/styles" "github.com/charmbracelet/crush/internal/update" @@ -481,6 +482,7 @@ func (app *App) setupEvents() { setupSubscriber(ctx, app.serviceEventsWG, "agent-notifications", app.agentNotifications.Subscribe, app.events) setupSubscriber(ctx, app.serviceEventsWG, "mcp", mcp.SubscribeEvents, app.events) setupSubscriber(ctx, app.serviceEventsWG, "lsp", SubscribeLSPEvents, app.events) + setupSubscriber(ctx, app.serviceEventsWG, "skills", skills.SubscribeEvents, app.events) cleanupFunc := func(context.Context) error { cancel() app.serviceEventsWG.Wait() diff --git a/internal/skills/skills.go b/internal/skills/skills.go index c14cdeb7231b8862841118dbee835c54dced1bd5..8dad22ec4982ed7369cbe70f7dd2b9a3f1f678b4 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -3,6 +3,7 @@ package skills import ( + "context" "errors" "fmt" "log/slog" @@ -15,6 +16,7 @@ import ( "sync" "github.com/charlievieth/fastwalk" + "github.com/charmbracelet/crush/internal/pubsub" "gopkg.in/yaml.v3" ) @@ -43,6 +45,36 @@ type Skill struct { Builtin bool `yaml:"-" json:"builtin"` } +// DiscoveryState represents the outcome of discovering a single skill file. +type DiscoveryState int + +const ( + // StateNormal indicates the skill was parsed and validated successfully. + StateNormal DiscoveryState = iota + // StateError indicates discovery encountered a scan/parse/validate error. + StateError +) + +// SkillState represents the latest discovery status of a skill file. +type SkillState struct { + Name string + Path string + State DiscoveryState + Err error +} + +// Event is published when skill discovery completes. +type Event struct { + States []*SkillState +} + +var broker = pubsub.NewBroker[Event]() + +// SubscribeEvents returns a channel that receives events when skill discovery state changes. +func SubscribeEvents(ctx context.Context) <-chan pubsub.Event[Event] { + return broker.Subscribe(ctx) +} + // Validate checks if the skill meets spec requirements. func (s *Skill) Validate() error { var errs []error @@ -141,8 +173,19 @@ func splitFrontmatter(content string) (frontmatter, body string, err error) { // Discover finds all valid skills in the given paths. func Discover(paths []string) []*Skill { var skills []*Skill + var states []*SkillState var mu sync.Mutex seen := make(map[string]bool) + addState := func(name, path string, state DiscoveryState, err error) { + mu.Lock() + states = append(states, &SkillState{ + Name: name, + Path: path, + State: state, + Err: err, + }) + mu.Unlock() + } for _, base := range paths { // We use fastwalk with Follow: true instead of filepath.WalkDir because @@ -156,6 +199,7 @@ func Discover(paths []string) []*Skill { err := fastwalk.Walk(&conf, base, func(path string, d os.DirEntry, err error) error { if err != nil { slog.Warn("Failed to walk skills path entry", "base", base, "path", path, "error", err) + addState("", path, StateError, err) return nil } if d.IsDir() || d.Name() != SkillFileName { @@ -171,16 +215,19 @@ func Discover(paths []string) []*Skill { skill, err := Parse(path) if err != nil { slog.Warn("Failed to parse skill file", "path", path, "error", err) + addState("", path, StateError, err) return nil } if err := skill.Validate(); err != nil { slog.Warn("Skill validation failed", "path", path, "error", err) + addState(skill.Name, path, StateError, err) return nil } slog.Debug("Successfully loaded skill", "name", skill.Name, "path", path) mu.Lock() skills = append(skills, skill) mu.Unlock() + addState(skill.Name, path, StateNormal, nil) return nil }) if err != nil { @@ -198,6 +245,7 @@ func Discover(paths []string) []*Skill { return left < right }) + broker.Publish(pubsub.UpdatedEvent, Event{States: states}) return skills } diff --git a/internal/skills/skills_test.go b/internal/skills/skills_test.go index 2c3abd3933b1f49735d14266d96897f5c3b896d3..e8ae7b72c8cec704229472c90f110994a55a0380 100644 --- a/internal/skills/skills_test.go +++ b/internal/skills/skills_test.go @@ -1,6 +1,7 @@ package skills import ( + "context" "os" "path/filepath" "strings" @@ -215,8 +216,7 @@ func TestSkillValidate(t *testing.T) { } func TestDiscover(t *testing.T) { - t.Parallel() - + // Not parallel: shares global broker with other Discover tests. tmpDir := t.TempDir() // Create valid skill 1. @@ -248,7 +248,31 @@ description: Name doesn't match directory. --- `), 0o644)) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + ch := SubscribeEvents(ctx) + skills := Discover([]string{tmpDir}) + + evt := <-ch + states := evt.Payload.States + var normalCount int + var errorCount int + var hasInvalidDir bool + for _, state := range states { + if state.State == StateNormal { + normalCount++ + } + if state.State == StateError { + errorCount++ + if strings.Contains(state.Path, "invalid-dir") { + hasInvalidDir = true + } + } + } + require.Equal(t, 2, normalCount) + require.Equal(t, 1, errorCount) + require.True(t, hasInvalidDir) require.Len(t, skills, 2) require.Equal(t, []string{"skill-two", "skill-one"}, []string{skills[0].Name, skills[1].Name}) @@ -260,6 +284,36 @@ description: Name doesn't match directory. require.True(t, names["skill-two"]) } +func TestDiscoverEmptyDir(t *testing.T) { + // Not parallel: shares global broker with other Discover tests. + + tmpDir := t.TempDir() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + ch := SubscribeEvents(ctx) + + skills := Discover([]string{tmpDir}) + + evt := <-ch + require.Empty(t, evt.Payload.States) + require.Empty(t, skills) +} + +func TestDiscoverMissingPath(t *testing.T) { + // Not parallel: shares global broker with other Discover tests. + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + ch := SubscribeEvents(ctx) + + skills := Discover([]string{filepath.Join(t.TempDir(), "missing")}) + + evt := <-ch + require.Empty(t, evt.Payload.States) + require.Empty(t, skills) +} + func TestToPromptXML(t *testing.T) { t.Parallel() diff --git a/internal/ui/model/landing.go b/internal/ui/model/landing.go index 0f3fc8d4d4012b953d2da0a3eb6b8a34d33fd535..eeebf5ffc302befee43c6450fcca04c8d5fbc2be 100644 --- a/internal/ui/model/landing.go +++ b/internal/ui/model/landing.go @@ -39,12 +39,13 @@ func (m *UI) landingView() string { layout.Fill(1), ).Split(m.layout.main).Assign(new(image.Rectangle), &remainingHeightArea) - mcpLspSectionWidth := min(30, (width-1)/2) + mcpLspSectionWidth := min(30, (width-2)/3) lspSection := m.lspInfo(mcpLspSectionWidth, max(1, remainingHeightArea.Dy()), false) mcpSection := m.mcpInfo(mcpLspSectionWidth, max(1, remainingHeightArea.Dy()), false) + skillsSection := m.skillsInfo(mcpLspSectionWidth, max(1, remainingHeightArea.Dy()), false) - content := lipgloss.JoinHorizontal(lipgloss.Left, lspSection, " ", mcpSection) + content := lipgloss.JoinHorizontal(lipgloss.Left, lspSection, " ", mcpSection, " ", skillsSection) return lipgloss.NewStyle(). Width(width). diff --git a/internal/ui/model/sidebar.go b/internal/ui/model/sidebar.go index 2b8db441a14495f9367963677bba4891a7fdd740..c0fa118d6bb1cdc8683eb84572d0ce7d62db77a5 100644 --- a/internal/ui/model/sidebar.go +++ b/internal/ui/model/sidebar.go @@ -56,51 +56,73 @@ func (m *UI) modelInfo(width int) string { return common.ModelInfo(m.com.Styles, modelName, providerName, reasoningInfo, modelContext, width) } -// getDynamicHeightLimits will give us the num of items to show in each section based on the hight +// getDynamicHeightLimits will give us the num of items to show in each section based on the height // some items are more important than others. -func getDynamicHeightLimits(availableHeight int) (maxFiles, maxLSPs, maxMCPs int) { +func getDynamicHeightLimits(availableHeight, fileCount, lspCount, mcpCount, skillCount int) (maxFiles, maxLSPs, maxMCPs, maxSkills int) { const ( - minItemsPerSection = 2 - defaultMaxFilesShown = 10 - defaultMaxLSPsShown = 8 - defaultMaxMCPsShown = 8 + minItemsPerSection = 2 + // Keep these high so dynamic layout uses available sidebar space + // instead of hitting small hard limits. + defaultMaxFilesShown = 1000 + defaultMaxLSPsShown = 1000 + defaultMaxMCPsShown = 1000 + defaultMaxSkillsShown = 1000 minAvailableHeightLimit = 10 ) - // If we have very little space, use minimum values if availableHeight < minAvailableHeightLimit { - return minItemsPerSection, minItemsPerSection, minItemsPerSection + return minItemsPerSection, minItemsPerSection, minItemsPerSection, minItemsPerSection } - // Distribute available height among the three sections - // Give priority to files, then LSPs, then MCPs - totalSections := 3 - heightPerSection := availableHeight / totalSections - - // Calculate limits for each section, ensuring minimums - maxFiles = max(minItemsPerSection, min(defaultMaxFilesShown, heightPerSection)) - maxLSPs = max(minItemsPerSection, min(defaultMaxLSPsShown, heightPerSection)) - maxMCPs = max(minItemsPerSection, min(defaultMaxMCPsShown, heightPerSection)) - - // If we have extra space, give it to files first - remainingHeight := availableHeight - (maxFiles + maxLSPs + maxMCPs) - if remainingHeight > 0 { - extraForFiles := min(remainingHeight, defaultMaxFilesShown-maxFiles) - maxFiles += extraForFiles - remainingHeight -= extraForFiles - - if remainingHeight > 0 { - extraForLSPs := min(remainingHeight, defaultMaxLSPsShown-maxLSPs) - maxLSPs += extraForLSPs - remainingHeight -= extraForLSPs - - if remainingHeight > 0 { - maxMCPs += min(remainingHeight, defaultMaxMCPsShown-maxMCPs) + maxFiles = minItemsPerSection + maxLSPs = minItemsPerSection + maxMCPs = minItemsPerSection + maxSkills = minItemsPerSection + + remainingHeight := max(0, availableHeight-(minItemsPerSection*4)) + + sectionValues := []*int{&maxFiles, &maxLSPs, &maxMCPs, &maxSkills} + sectionCaps := []int{defaultMaxFilesShown, defaultMaxLSPsShown, defaultMaxMCPsShown, defaultMaxSkillsShown} + sectionNeeds := []int{max(0, fileCount-maxFiles), max(0, lspCount-maxLSPs), max(0, mcpCount-maxMCPs), max(0, skillCount-maxSkills)} + + for remainingHeight > 0 { + allocated := false + for i, section := range sectionValues { + if remainingHeight == 0 { + break + } + if sectionNeeds[i] == 0 || *section >= sectionCaps[i] { + continue + } + *section = *section + 1 + sectionNeeds[i]-- + remainingHeight-- + allocated = true + } + if !allocated { + break + } + } + + for remainingHeight > 0 { + allocated := false + for i, section := range sectionValues { + if remainingHeight == 0 { + break + } + if *section >= sectionCaps[i] { + continue } + *section = *section + 1 + remainingHeight-- + allocated = true + } + if !allocated { + break } } - return maxFiles, maxLSPs, maxMCPs + return maxFiles, maxLSPs, maxMCPs, maxSkills } // sidebar renders the chat sidebar containing session title, working @@ -142,11 +164,31 @@ func (m *UI) drawSidebar(scr uv.Screen, area uv.Rectangle) { layout.Len(lipgloss.Height(sidebarHeader)), layout.Fill(1), ).Split(m.layout.sidebar).Assign(new(image.Rectangle), &remainingHeightArea) - remainingHeight := remainingHeightArea.Dy() - 10 - maxFiles, maxLSPs, maxMCPs := getDynamicHeightLimits(remainingHeight) + remainingHeight := remainingHeightArea.Dy() - 6 + filesCount := 0 + for _, f := range m.sessionFiles { + if f.Additions == 0 && f.Deletions == 0 { + continue + } + filesCount++ + } + + lspsCount := len(m.lspStates) + + mcpsCount := 0 + for _, mcpCfg := range m.com.Config().MCP.Sorted() { + if _, ok := m.mcpStates[mcpCfg.Name]; ok { + mcpsCount++ + } + } + + skillsCount := len(m.skillStatusItems()) + + maxFiles, maxLSPs, maxMCPs, maxSkills := getDynamicHeightLimits(remainingHeight, filesCount, lspsCount, mcpsCount, skillsCount) lspSection := m.lspInfo(width, maxLSPs, true) mcpSection := m.mcpInfo(width, maxMCPs, true) + skillsSection := m.skillsInfo(width, maxSkills, true) filesSection := m.filesInfo(m.com.Workspace.WorkingDir(), width, maxFiles, true) uv.NewStyledString( @@ -162,6 +204,8 @@ func (m *UI) drawSidebar(scr uv.Screen, area uv.Rectangle) { lspSection, "", mcpSection, + "", + skillsSection, ), ), ).Draw(scr, area) diff --git a/internal/ui/model/skills.go b/internal/ui/model/skills.go new file mode 100644 index 0000000000000000000000000000000000000000..cf4417c8977e836ab8daf8a99aaeb45c2413a6bc --- /dev/null +++ b/internal/ui/model/skills.go @@ -0,0 +1,127 @@ +package model + +import ( + "fmt" + "path/filepath" + "slices" + "strings" + "sync" + + "charm.land/lipgloss/v2" + "github.com/charmbracelet/crush/internal/skills" + "github.com/charmbracelet/crush/internal/ui/common" + "github.com/charmbracelet/crush/internal/ui/styles" +) + +type skillStatusItem struct { + icon string + name string + title string + // description is reserved for future use (e.g. showing error details). + description string +} + +var builtinSkillsCache struct { + once sync.Once + skills []*skills.Skill +} + +func cachedBuiltinSkills() []*skills.Skill { + builtinSkillsCache.once.Do(func() { + builtinSkillsCache.skills = skills.DiscoverBuiltin() + }) + return builtinSkillsCache.skills +} + +// skillsInfo renders the skill discovery status section showing loaded and +// invalid skills. +func (m *UI) skillsInfo(width, maxItems int, isSection bool) string { + t := m.com.Styles + + title := t.ResourceGroupTitle.Render("Skills") + if isSection { + title = common.Section(t, title, width) + } + + items := m.skillStatusItems() + if len(items) == 0 { + list := t.ResourceAdditionalText.Render("None") + return lipgloss.NewStyle().Width(width).Render(fmt.Sprintf("%s\n\n%s", title, list)) + } + + list := skillsList(t, items, width, maxItems) + return lipgloss.NewStyle().Width(width).Render(fmt.Sprintf("%s\n\n%s", title, list)) +} + +func (m *UI) skillStatusItems() []skillStatusItem { + t := m.com.Styles + var items []skillStatusItem + stateNames := make(map[string]struct{}, len(m.skillStates)) + + states := slices.Clone(m.skillStates) + slices.SortStableFunc(states, func(a, b *skills.SkillState) int { + return strings.Compare(a.Path, b.Path) + }) + for _, state := range states { + name := state.Name + if name == "" { + name = filepath.Base(filepath.Dir(state.Path)) + } + stateNames[name] = struct{}{} + icon := t.ResourceOnlineIcon.String() + if state.State == skills.StateError { + icon = t.ResourceErrorIcon.String() + } + items = append(items, skillStatusItem{ + icon: icon, + name: name, + title: t.ResourceName.Render(name), + }) + } + + builtin := cachedBuiltinSkills() + slices.SortStableFunc(builtin, func(a, b *skills.Skill) int { + return strings.Compare(a.Name, b.Name) + }) + for _, skill := range builtin { + if _, ok := stateNames[skill.Name]; ok { + continue + } + items = append(items, skillStatusItem{ + icon: t.ResourceOnlineIcon.String(), + name: skill.Name, + title: t.ResourceName.Render(skill.Name), + }) + } + + slices.SortStableFunc(items, func(a, b skillStatusItem) int { + return strings.Compare(a.name, b.name) + }) + + return items +} + +func skillsList(t *styles.Styles, items []skillStatusItem, width, maxItems int) string { + if maxItems <= 0 { + return "" + } + + if len(items) > maxItems { + visibleItems := items[:maxItems-1] + remaining := len(items) - (maxItems - 1) + items = append(visibleItems, skillStatusItem{ + name: "more", + title: t.ResourceAdditionalText.Render(fmt.Sprintf("…and %d more", remaining)), + }) + } + + renderedItems := make([]string, 0, len(items)) + for _, item := range items { + renderedItems = append(renderedItems, common.Status(t, common.StatusOpts{ + Icon: item.icon, + Title: item.title, + Description: item.description, + }, width)) + } + return lipgloss.JoinVertical(lipgloss.Left, renderedItems...) +} diff --git a/internal/ui/model/skills_test.go b/internal/ui/model/skills_test.go new file mode 100644 index 0000000000000000000000000000000000000000..7ca2fb9299620261a79ff2c2aa6d9db81b4cbbd7 --- /dev/null +++ b/internal/ui/model/skills_test.go @@ -0,0 +1,58 @@ +package model + +import ( + "testing" + + "github.com/charmbracelet/crush/internal/skills" + "github.com/charmbracelet/crush/internal/ui/common" + uistyles "github.com/charmbracelet/crush/internal/ui/styles" + "github.com/stretchr/testify/require" +) + +// TestSkillStatusItemsIncludesBuiltinSkills verifies sidebar skills include +// both runtime-discovered skill states and builtin skills that may not have +// emitted a SkillState event yet. +func TestSkillStatusItemsIncludesBuiltinSkills(t *testing.T) { + t.Parallel() + + st := uistyles.DefaultStyles() + ui := &UI{ + com: &common.Common{Styles: &st}, + skillStates: []*skills.SkillState{ + {Name: "go-doc", Path: "/tmp/go-doc/SKILL.md", State: skills.StateNormal}, + }, + } + + items := ui.skillStatusItems() + require.NotEmpty(t, items) + + var hasGoDoc bool + for _, item := range items { + if item.title == st.ResourceName.Render("go-doc") { + hasGoDoc = true + break + } + } + require.True(t, hasGoDoc) + + builtinSkills := skills.DiscoverBuiltin() + require.NotEmpty(t, builtinSkills) + + var hasBuiltin bool + for _, skill := range builtinSkills { + if skill.Name == "go-doc" { + continue + } + expected := st.ResourceName.Render(skill.Name) + for _, item := range items { + if item.title == expected { + hasBuiltin = true + break + } + } + if hasBuiltin { + break + } + } + require.True(t, hasBuiltin) +} diff --git a/internal/ui/model/ui.go b/internal/ui/model/ui.go index 5d5cb3d769e46249ab09d4df85689ca143a062ea..779865bbd100e0f8c0aa7ad6106bc72a860921d3 100644 --- a/internal/ui/model/ui.go +++ b/internal/ui/model/ui.go @@ -38,6 +38,7 @@ import ( "github.com/charmbracelet/crush/internal/permission" "github.com/charmbracelet/crush/internal/pubsub" "github.com/charmbracelet/crush/internal/session" + "github.com/charmbracelet/crush/internal/skills" "github.com/charmbracelet/crush/internal/ui/anim" "github.com/charmbracelet/crush/internal/ui/attachments" "github.com/charmbracelet/crush/internal/ui/chat" @@ -220,6 +221,9 @@ type UI struct { // mcp mcpStates map[string]mcp.ClientInfo + // skills + skillStates []*skills.SkillState + // sidebarLogo keeps a cached version of the sidebar sidebarLogo. sidebarLogo string @@ -619,6 +623,8 @@ func (m *UI) Update(msg tea.Msg) (tea.Model, tea.Cmd) { cmds = append(cmds, m.handleFileEvent(msg.Payload)) case pubsub.Event[app.LSPEvent]: m.lspStates = app.GetLSPStates() + case pubsub.Event[skills.Event]: + m.skillStates = msg.Payload.States case pubsub.Event[mcp.Event]: switch msg.Payload.Type { case mcp.EventStateChanged: @@ -3535,13 +3541,14 @@ func (m *UI) drawSessionDetails(scr uv.Screen, area uv.Rectangle) { remainingHeight := height - lipgloss.Height(detailsHeader) - lipgloss.Height(version) const maxSectionWidth = 50 - sectionWidth := min(maxSectionWidth, width/3-2) // account for 2 spaces - maxItemsPerSection := remainingHeight - 3 // Account for section title and spacing + sectionWidth := max(1, min(maxSectionWidth, width/4-2)) // account for spacing between sections + maxItemsPerSection := remainingHeight - 3 // Account for section title and spacing lspSection := m.lspInfo(sectionWidth, maxItemsPerSection, false) mcpSection := m.mcpInfo(sectionWidth, maxItemsPerSection, false) + skillsSection := m.skillsInfo(sectionWidth, maxItemsPerSection, false) filesSection := m.filesInfo(m.com.Workspace.WorkingDir(), sectionWidth, maxItemsPerSection, false) - sections := lipgloss.JoinHorizontal(lipgloss.Top, filesSection, " ", lspSection, " ", mcpSection) + sections := lipgloss.JoinHorizontal(lipgloss.Top, filesSection, " ", lspSection, " ", mcpSection, " ", skillsSection) uv.NewStyledString( s.CompactDetails.View. Width(area.Dx()).