Skip to content

Commit d53dd5b

Browse files
authored
feat(skills): launch Skills as default landing route (#2130)
* feat(skills): always render Skills nav link and drop flag-specific copy Removes the useFeatureFlag(SKILLS) check in the top nav so the Skills entry is unconditionally rendered (now the first item, matching its new role as the landing route), and drops the dead Skills-specific name/description branches in useExperimentalFeatures — the flag was never listed there (isExperimental: false) and the branches become unreachable once the key is removed in the next commit. Adds a top-nav test asserting the Skills link renders without any flag setup. * feat(skills): remove SKILLS feature flag Skills is now GA. Drops the SKILLS key from the shared featureFlagKeys map and its options block in the main-process flag store. Existing 'feature_flag_skills' rows in SQLite become inert orphans — getFeatureFlag is only called for keys still present in featureFlagKeys, so no migration is needed. Reroutes the four SKILLS-specific assertions in flags.test.ts onto featureFlagKeys.AGENTS (same shape: non-disabled, default false) so the SQLite read/write/default paths stay covered. * feat(skills): make Skills the default landing route Switches the '/' redirect from '/group/default' to '/skills'. The Skills route's validateSearch already defaults tab to 'installed' when the search param is missing, so first launch lands directly on the Installed tab without needing extra search params. Adds renderer/src/routes/__tests__/index.redirect.test.tsx covering the new redirect using the same beforeLoad-driven router pattern as playground.index.test.tsx. * test(e2e): navigate to MCP Servers before creating test groups The shared Playwright fixture and the MCP Optimizer cleanup spec both clicked 'Add a group' as their first UI action, relying on the app landing on /group/default. Now that /skills is the default landing route, that button is no longer on screen and every group-creating spec timed out waiting for it. Both helpers now click the MCP Servers nav link first, then proceed with the existing dialog flow. This is a minimal, behaviour-preserving fix — no specs needed individual changes because they all run inside the test group the fixture creates. * test(e2e): cover Skills default route and tab navigation Adds e2e-tests/skills.spec.ts with two complementary tests: 1. 'app launches into the Skills page on /skills' uses launchApp directly (no shared fixture, no group setup) so the assertion targets the app's actual initial route. Asserts all three tabs render and the Installed empty state is the visible panel, proving the new '/' -> '/skills?tab=installed' redirect. 2. 'switches between tabs and shows the right empty state' uses the shared fixture and walks Installed -> Local Builds -> Registry from the unconditional Skills nav entry, implicitly covering both the removed feature flag and the page's tab UX.
1 parent d0a04a1 commit d53dd5b

11 files changed

Lines changed: 151 additions & 39 deletions

File tree

e2e-tests/fixtures/electron.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,9 @@ export function deleteTestSecretViaCli(): void {
7777
}
7878

7979
async function createAndActivateTestGroup(window: Page): Promise<void> {
80+
// The app now lands on /skills by default — navigate to the MCP Servers
81+
// groups page where the "Add a group" affordance lives.
82+
await window.getByRole('link', { name: /mcp servers/i }).click()
8083
await window.getByRole('button', { name: /add a group/i }).click()
8184
await window.getByRole('dialog').waitFor()
8285
await window.getByLabel(/name/i).fill(TEST_GROUP_NAME)

e2e-tests/mcp-optimizer-startup-cleanup.spec.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,9 @@ async function createGroupViaUi(
4646
groupName: string
4747
): Promise<void> {
4848
const { window } = launched
49+
// The app now lands on /skills by default — navigate to the MCP Servers
50+
// groups page where the "Add a group" affordance lives.
51+
await window.getByRole('link', { name: /mcp servers/i }).click()
4952
await window.getByRole('button', { name: /add a group/i }).click()
5053
await window.getByRole('dialog').waitFor()
5154
await window.getByLabel(/name/i).fill(groupName)

e2e-tests/skills.spec.ts

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
import path from 'path'
2+
import fs from 'fs'
3+
import os from 'os'
4+
import { test as base, expect } from '@playwright/test'
5+
import { test } from './fixtures/electron'
6+
import { launchApp } from './helpers/app-relaunch'
7+
8+
// Standalone test (does not use the shared fixture) so the assertion runs
9+
// against the app's true initial route, before any nav clicks happen.
10+
base.describe('Skills default landing route', () => {
11+
base('app launches into the Skills page on /skills', async () => {
12+
const userDataDir = fs.mkdtempSync(
13+
path.join(os.tmpdir(), 'toolhive-e2e-skills-default-')
14+
)
15+
const launched = await launchApp(userDataDir)
16+
try {
17+
// Tabs are part of the Skills page only, so their presence proves we
18+
// landed on /skills rather than /group/default.
19+
await expect(
20+
launched.window.getByRole('tab', { name: 'Registry' })
21+
).toBeVisible()
22+
await expect(
23+
launched.window.getByRole('tab', { name: 'Installed' })
24+
).toBeVisible()
25+
await expect(
26+
launched.window.getByRole('tab', { name: 'Local Builds' })
27+
).toBeVisible()
28+
29+
// Installed is the default tab — empty state confirms its panel is the
30+
// active one on a clean userDataDir.
31+
await expect(
32+
launched.window.getByRole('heading', { name: /no skills installed/i })
33+
).toBeVisible()
34+
} finally {
35+
await launched.close()
36+
fs.rmSync(userDataDir, { recursive: true, force: true })
37+
}
38+
})
39+
})
40+
41+
test.describe('Skills page navigation', () => {
42+
test('switches between tabs and shows the right empty state', async ({
43+
window,
44+
}) => {
45+
// The shared fixture leaves us on the test group page; navigate to Skills.
46+
await window.getByRole('link', { name: /^skills$/i }).click()
47+
48+
// Installed (default) shows the no-skills empty state.
49+
await expect(
50+
window.getByRole('heading', { name: /no skills installed/i })
51+
).toBeVisible()
52+
53+
// Local Builds shows its own empty state with a "Build skill" CTA.
54+
await window.getByRole('tab', { name: 'Local Builds' }).click()
55+
await expect(
56+
window.getByRole('heading', { name: /no local builds/i })
57+
).toBeVisible()
58+
await expect(
59+
window.getByRole('button', { name: /build skill/i })
60+
).toBeVisible()
61+
62+
// Registry tab renders its toolbar (search input + view toggle).
63+
await window.getByRole('tab', { name: 'Registry' }).click()
64+
await expect(window.getByPlaceholder(/search/i).first()).toBeVisible()
65+
})
66+
})

main/src/feature-flags/__tests__/flags.test.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -71,14 +71,14 @@ describe('Feature Flags', () => {
7171
it('should return value from SQLite when available', () => {
7272
vi.mocked(readFeatureFlag).mockReturnValueOnce(true)
7373

74-
const result = getFeatureFlag(featureFlagKeys.SKILLS)
74+
const result = getFeatureFlag(featureFlagKeys.AGENTS)
7575
expect(result).toBe(true)
7676
})
7777

7878
it('should return false (default) when SQLite returns undefined', () => {
7979
vi.mocked(readFeatureFlag).mockReturnValueOnce(undefined)
8080

81-
const result = getFeatureFlag(featureFlagKeys.SKILLS)
81+
const result = getFeatureFlag(featureFlagKeys.AGENTS)
8282
expect(result).toBe(false)
8383
})
8484

@@ -87,7 +87,7 @@ describe('Feature Flags', () => {
8787
throw new Error('DB error')
8888
})
8989

90-
const result = getFeatureFlag(featureFlagKeys.SKILLS)
90+
const result = getFeatureFlag(featureFlagKeys.AGENTS)
9191
expect(result).toBe(false)
9292
})
9393

@@ -136,10 +136,10 @@ describe('Feature Flags', () => {
136136

137137
describe('enableFeatureFlag', () => {
138138
it('should write true to SQLite for enabled flags', () => {
139-
enableFeatureFlag(featureFlagKeys.SKILLS)
139+
enableFeatureFlag(featureFlagKeys.AGENTS)
140140

141141
expect(writeFeatureFlag).toHaveBeenCalledWith(
142-
`feature_flag_${featureFlagKeys.SKILLS}`,
142+
`feature_flag_${featureFlagKeys.AGENTS}`,
143143
true
144144
)
145145
})

main/src/feature-flags/flags.ts

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,6 @@ const featureFlagOptions: Record<FeatureFlagKey, FeatureFlagOptions> = {
1515
defaultValue: false,
1616
isExperimental: true,
1717
},
18-
[featureFlagKeys.SKILLS]: {
19-
isDisabled: false,
20-
defaultValue: false,
21-
isExperimental: false,
22-
},
2318
[featureFlagKeys.AGENTS]: {
2419
isDisabled: false,
2520
defaultValue: false,

renderer/src/common/components/layout/top-nav/__tests__/top-nav.test.tsx

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,13 @@ describe('TopNav', () => {
3333
return renderRoute(router, { permissions })
3434
}
3535

36+
it('renders Skills navigation link', async () => {
37+
renderTopNav()
38+
await waitFor(() => {
39+
expect(screen.getByText('Skills')).toBeInTheDocument()
40+
})
41+
})
42+
3643
it('renders MCP Servers navigation link', async () => {
3744
renderTopNav()
3845
await waitFor(() => {

renderer/src/common/components/layout/top-nav/index.tsx

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,6 @@ import { getOsDesignVariant } from '@/common/lib/os-design'
2828
import { trackEvent } from '@/common/lib/analytics'
2929
import { NavSeparator } from './nav-separator'
3030
import { NavIconButton } from './nav-icon-button'
31-
import { useFeatureFlag } from '@/common/hooks/use-feature-flag'
32-
import { featureFlagKeys } from '@utils/feature-flags'
3331
import { usePermissions } from '@/common/contexts/permissions'
3432
import { PERMISSION_KEYS } from '@/common/contexts/permissions/permission-keys'
3533
import { buildOnrampDocsUrl } from '@/common/lib/onramp-url'
@@ -69,23 +67,20 @@ function useIsActive() {
6967

7068
function TopNavLinks() {
7169
const isActive = useIsActive()
72-
const isSkillsEnabled = useFeatureFlag(featureFlagKeys.SKILLS)
7370
const { canShow } = usePermissions()
7471

7572
return (
7673
<NavigationMenu>
7774
<NavigationMenuList className="gap-2">
78-
{isSkillsEnabled && (
79-
<NavigationMenuItem>
80-
<NavButton
81-
to="/skills"
82-
icon={BookOpen}
83-
isActive={isActive(['/skills'])}
84-
>
85-
Skills
86-
</NavButton>
87-
</NavigationMenuItem>
88-
)}
75+
<NavigationMenuItem>
76+
<NavButton
77+
to="/skills"
78+
icon={BookOpen}
79+
isActive={isActive(['/skills'])}
80+
>
81+
Skills
82+
</NavButton>
83+
</NavigationMenuItem>
8984
<NavigationMenuItem>
9085
<NavButton
9186
to="/group/default"

renderer/src/common/hooks/use-experimental-features.tsx

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'
22
import log from 'electron-log/renderer'
33
import { useCallback } from 'react'
4-
import { featureFlagKeys } from '../../../../utils/feature-flags'
54
import { trackEvent } from '../lib/analytics'
65

76
interface FeatureFlag {
@@ -12,21 +11,13 @@ interface FeatureFlag {
1211
}
1312

1413
function formatFeatureFlagName(key: string): string {
15-
if (key === featureFlagKeys.SKILLS) {
16-
return 'Skills'
17-
}
18-
1914
return key
2015
.split('_')
2116
.map((word) => word.charAt(0).toUpperCase() + word.slice(1).toLowerCase())
2217
.join(' ')
2318
}
2419

2520
function formatFeatureFlagDescription(key: string): React.ReactNode {
26-
if (key === featureFlagKeys.SKILLS) {
27-
return 'Browse and manage AI agent skills installed via ToolHive.'
28-
}
29-
3021
return `Enable ${formatFeatureFlagName(key)} feature`
3122
}
3223

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
import { describe, it, expect } from 'vitest'
2+
import { waitFor } from '@testing-library/react'
3+
import {
4+
createMemoryHistory,
5+
createRootRoute,
6+
createRoute,
7+
Outlet,
8+
Router,
9+
} from '@tanstack/react-router'
10+
import { renderRoute } from '@/common/test/render-route'
11+
import { createTestRouter } from '@/common/test/create-test-router'
12+
import { Route as IndexRoute } from '../index'
13+
14+
function renderIndexRoute() {
15+
const beforeLoad = IndexRoute.options.beforeLoad as unknown as (
16+
...args: unknown[]
17+
) => Promise<void> | void
18+
19+
const rootRoute = createRootRoute({
20+
component: Outlet,
21+
})
22+
23+
const indexRoute = createRoute({
24+
getParentRoute: () => rootRoute,
25+
path: '/',
26+
component: () => null,
27+
beforeLoad,
28+
})
29+
30+
const skillsRoute = createRoute({
31+
getParentRoute: () => rootRoute,
32+
path: '/skills',
33+
component: () => <div data-testid="skills-route" />,
34+
})
35+
36+
const router = new Router({
37+
routeTree: rootRoute.addChildren([indexRoute, skillsRoute]),
38+
history: createMemoryHistory({ initialEntries: ['/'] }),
39+
defaultNotFoundComponent: () => null,
40+
})
41+
42+
const utils = renderRoute(
43+
router as unknown as ReturnType<typeof createTestRouter>
44+
)
45+
46+
return { ...utils, router }
47+
}
48+
49+
describe('Index route (/)', () => {
50+
it('redirects to /skills', async () => {
51+
const { router } = renderIndexRoute()
52+
53+
await waitFor(() => {
54+
expect(router.state.location.pathname).toBe('/skills')
55+
})
56+
})
57+
})

renderer/src/routes/index.tsx

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,6 @@ import { createFileRoute, redirect } from '@tanstack/react-router'
22

33
export const Route = createFileRoute('/')({
44
beforeLoad: () => {
5-
throw redirect({
6-
to: '/group/$groupName',
7-
params: { groupName: 'default' },
8-
replace: true,
9-
})
5+
throw redirect({ to: '/skills', replace: true })
106
},
117
})

0 commit comments

Comments
 (0)