Skip to content

Commit 6fecc4c

Browse files
committed
SEP-1270: Fix MySQL/Archive sidebar links and route the sidebar through shared ROUTES
The React shell sidebar hardcoded `to:` paths that drifted from router.tsx: - MySQL → /backups/mysql resolved to PlaceholderPage (real UI is /plugins/mysql_backups, per the plugin's PLUGIN_BASE_PATH) - Archive → /archive had no route and fell through to NotFoundPage (real UI is /plugins/archives) Fix: - navigation.tsx now sources every `to:` from the shared ROUTES map instead of hardcoded strings, and documents the URL convention (schema-driven plugins under /plugins/<name>, domain-grouped backups under /backups/<db>, cross-cutting tools at bare top-level paths). - shared ROUTES: correct the MySQL/Archive values, rename the misleading backupsMysql key to mysqlBackups, add checksums/atw/dipper entries, and clarify that router.tsx keeps its own literals (alignment is detected by the e2e net, not enforced at compile time). - router.tsx: remove the now-dead /backups/mysql placeholder route. Audit of every defaultNavItems entry against router.tsx confirmed the rest resolve correctly (PostgreSQL now routes to BackupPgPlugin shipped in SEP-1266); the remaining PlaceholderPage entries are intentional. Add a Playwright sidebar-navigation spec that clicks each non-placeholder entry and asserts the URL plus a positive sentinel rendered by the target plugin before checking the page is not the placeholder/404 — the regression net that would have caught both bugs at review time.
1 parent 0c32e62 commit 6fecc4c

4 files changed

Lines changed: 307 additions & 19 deletions

File tree

Lines changed: 258 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,258 @@
1+
/**
2+
* Copyright (C) 2026 Percona LLC
3+
*
4+
* This program is free software: you can redistribute it and/or modify
5+
* it under the terms of the GNU Affero General Public License as published by
6+
* the Free Software Foundation, either version 3 of the License, or
7+
* (at your option) any later version.
8+
*
9+
* This program is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12+
* GNU Affero General Public License for more details.
13+
*
14+
* You should have received a copy of the GNU Affero General Public License
15+
* along with this program. If not, see <https://www.gnu.org/licenses/>.
16+
*/
17+
18+
/**
19+
* Sidebar wiring regression net (SEP-1270).
20+
*
21+
* The per-plugin specs (mysql-backups, archives, …) verify each plugin page in
22+
* isolation by navigating directly to its URL. They do NOT exercise the sidebar
23+
* itself, which is how two regressions slipped through review: the MySQL entry
24+
* pointed at /backups/mysql (PlaceholderPage) and the Archive entry at /archive
25+
* (no route → NotFoundPage).
26+
*
27+
* This spec clicks every non-placeholder sidebar entry the way a user would and
28+
* asserts (1) the resulting URL, (2) a positive sentinel rendered by the target
29+
* plugin, and only then (3) that neither the "under construction" PlaceholderPage
30+
* nor the 404 NotFoundPage is showing. The positive sentinel is essential: pages
31+
* are lazy()/Suspense-loaded, so the URL flips synchronously on click while the
32+
* chunk is still resolving — asserting placeholder/404 *absence* against that
33+
* still-loading DOM would pass even for a regressed route. Waiting for the
34+
* target's own element first guarantees the page has actually mounted.
35+
*
36+
* Entries intentionally excluded (they still route to PlaceholderPage by design):
37+
* Alerts/Templates, Schema Change/Alters, Reports, Settings.
38+
*/
39+
40+
import { test, expect, type Locator, type Page } from '@playwright/test';
41+
42+
// ── Mocks ───────────────────────────────────────────────────────────────────
43+
// NOTE: this auth+API mock is intentionally close to the one in shell.spec.ts.
44+
// Per _template.spec.ts, the shared base ought to be extracted to
45+
// tests/helpers/mock-apis.ts once a consumer needs it; tracked as a fast-follow
46+
// so this ticket stays scoped to the sidebar fix.
47+
48+
const MOCK_TOKEN = { access_token: 'smoke-test-token', expires_in: 3600 };
49+
50+
const MOCK_USER = {
51+
id: '00000000-0000-0000-0000-000000000001',
52+
username: 'smoke',
53+
email: 'smoke@percona.com',
54+
firstName: 'Smoke',
55+
lastName: 'Test',
56+
isAdmin: false,
57+
};
58+
59+
// Heading served for schema-driven plugins whose display name we don't assert
60+
// individually (the URL already identifies them). Schema-driven pages render
61+
// `schema.display_name` as their h4, so this is a deterministic sentinel.
62+
const GENERIC_PLUGIN_HEADING = 'SEP Plugin';
63+
64+
// Plugins whose display-name heading we assert explicitly (keyed by the
65+
// `<name>` in /api/plugins/<name>/schema). These are the schema-driven entries
66+
// this ticket is about — including the two that regressed.
67+
const SCHEMA_DISPLAY_NAMES: Record<string, string> = {
68+
checksums: 'Checksums',
69+
mysql_backups: 'MySQL Backups',
70+
archives: 'Archives',
71+
};
72+
73+
async function mockAuthenticatedApis(page: Page): Promise<void> {
74+
await page.route('**/api/**', (route) => {
75+
const { pathname } = new URL(route.request().url());
76+
77+
// Pass through Vite's internal module-serving paths (e.g. /@fs/...)
78+
if (!pathname.startsWith('/api/')) {
79+
return route.continue();
80+
}
81+
82+
if (pathname.includes('/oauth/refresh')) {
83+
return route.fulfill({
84+
status: 200,
85+
contentType: 'application/json',
86+
body: JSON.stringify(MOCK_TOKEN),
87+
});
88+
}
89+
90+
if (pathname.includes('/users/me')) {
91+
return route.fulfill({
92+
status: 200,
93+
contentType: 'application/json',
94+
body: JSON.stringify(MOCK_USER),
95+
});
96+
}
97+
98+
const schemaMatch = pathname.match(/\/api\/plugins\/(.+)\/schema$/);
99+
if (schemaMatch) {
100+
const name = schemaMatch[1];
101+
return route.fulfill({
102+
status: 200,
103+
contentType: 'application/json',
104+
body: JSON.stringify({
105+
name,
106+
display_name: SCHEMA_DISPLAY_NAMES[name] ?? GENERIC_PLUGIN_HEADING,
107+
capabilities: { chaining: false, alert_on_fail: false, scheduling: false, stats: false },
108+
forms: [],
109+
list_view: { columns: [] },
110+
}),
111+
});
112+
}
113+
114+
if (pathname.endsWith('/sep/dashboard/')) {
115+
return route.fulfill({
116+
status: 200,
117+
contentType: 'application/json',
118+
body: JSON.stringify({ nodes: 0, tasks: 0, snippets: 0, targets: 0 }),
119+
});
120+
}
121+
122+
if (pathname.includes('/tasks/history/')) {
123+
return route.fulfill({
124+
status: 200,
125+
contentType: 'application/json',
126+
body: JSON.stringify({ items: [], total: 0, offset: 0, limit: 5 }),
127+
});
128+
}
129+
130+
// Default: empty success for plugin task lists and anything else
131+
return route.fulfill({
132+
status: 200,
133+
contentType: 'application/json',
134+
body: '[]',
135+
});
136+
});
137+
}
138+
139+
// ── Sidebar map ───────────────────────────────────────────────────────────────
140+
// One entry per non-placeholder leaf in shell/src/contexts/navigation.tsx.
141+
// `group` — collapsible parent that must be expanded before the child shows.
142+
// `urlPattern` — matched against the post-navigation URL (plugins may redirect
143+
// to a default sub-route, e.g. /backups/mongodb → /backups/mongodb/backups).
144+
// `sentinel` — positive locator the target page renders; asserted before the
145+
// placeholder/404 negative checks so we never assert against a
146+
// still-loading DOM.
147+
interface SidebarTarget {
148+
label: string;
149+
group?: string;
150+
urlPattern: RegExp;
151+
sentinel: (page: Page) => Locator;
152+
}
153+
154+
const heading = (name: string) => (page: Page) => page.getByRole('heading', { name }).first();
155+
156+
const TARGETS: SidebarTarget[] = [
157+
{
158+
label: 'Inventory',
159+
urlPattern: /\/inventory(\/|$)/,
160+
sentinel: heading(GENERIC_PLUGIN_HEADING),
161+
},
162+
{ label: 'Tasks', urlPattern: /\/tasks(\/|$)/, sentinel: heading(GENERIC_PLUGIN_HEADING) },
163+
{ label: 'Snippets', urlPattern: /\/snippets(\/|$)/, sentinel: heading('Snippet Manager') },
164+
{
165+
label: 'Collect Diagnostic Data',
166+
urlPattern: /\/atw(\/|$)/,
167+
sentinel: heading('Collect Diagnostic Data'),
168+
},
169+
{ label: 'Checksums', urlPattern: /\/plugins\/checksums(\/|$)/, sentinel: heading('Checksums') },
170+
{
171+
label: 'Troubleshooting',
172+
group: 'Alerts',
173+
urlPattern: /\/alerts\/troubleshooting(\/|$)/,
174+
// Empty-state page renders no heading — assert its empty-state copy instead.
175+
sentinel: (page) => page.getByText(/No alerts found/i),
176+
},
177+
{
178+
label: 'MySQL',
179+
group: 'Backups',
180+
urlPattern: /\/plugins\/mysql_backups(\/|$)/,
181+
sentinel: heading('MySQL Backups'),
182+
},
183+
{
184+
label: 'MongoDB',
185+
group: 'Backups',
186+
urlPattern: /\/backups\/mongodb(\/|$)/,
187+
sentinel: heading(GENERIC_PLUGIN_HEADING),
188+
},
189+
{
190+
label: 'PostgreSQL',
191+
group: 'Backups',
192+
urlPattern: /\/backups\/postgresql(\/|$)/,
193+
sentinel: heading(GENERIC_PLUGIN_HEADING),
194+
},
195+
{ label: 'Archive', urlPattern: /\/plugins\/archives(\/|$)/, sentinel: heading('Archives') },
196+
{
197+
label: 'Dipper Data Collection',
198+
urlPattern: /\/dipper(\/|$)/,
199+
sentinel: heading(GENERIC_PLUGIN_HEADING),
200+
},
201+
];
202+
203+
// PlaceholderPage / NotFoundPage sentinel copy — their presence means the
204+
// sidebar landed on a broken (unmigrated / unrouted) destination.
205+
const PLACEHOLDER_TEXT = /implemented during the frontend migration/i;
206+
const NOT_FOUND_TEXT = /Page not found/i;
207+
208+
const LAZY_TIMEOUT = 30_000;
209+
210+
test.describe('sidebar navigation wiring', () => {
211+
test.beforeEach(async ({ page }) => {
212+
await mockAuthenticatedApis(page);
213+
await page.goto('/');
214+
// Wait for the authenticated shell (sidebar) before clicking around.
215+
await expect(page.getByRole('heading', { name: 'Dashboard' })).toBeVisible({
216+
timeout: LAZY_TIMEOUT,
217+
});
218+
});
219+
220+
for (const target of TARGETS) {
221+
test(`sidebar → ${target.group ? `${target.group} / ` : ''}${target.label} mounts its plugin`, async ({
222+
page,
223+
}) => {
224+
// Expand the parent group so its children render (Collapse uses unmountOnExit).
225+
if (target.group) {
226+
const child = page.getByRole('button', { name: target.label });
227+
if (!(await child.isVisible().catch(() => false))) {
228+
await page.getByRole('button', { name: target.group }).click();
229+
}
230+
}
231+
232+
await page.getByRole('button', { name: target.label }).click();
233+
234+
// URL resolves to the target plugin's route (not a placeholder / 404 path).
235+
await expect(page).toHaveURL(target.urlPattern, { timeout: LAZY_TIMEOUT });
236+
237+
// Positive sentinel: wait until the target page has actually mounted. This
238+
// must come BEFORE the negative checks below, otherwise they race the
239+
// lazy chunk and pass against a still-loading DOM.
240+
await expect(target.sentinel(page)).toBeVisible({ timeout: LAZY_TIMEOUT });
241+
242+
// Negative sentinels: the broken destinations render these, the real ones never do.
243+
await expect(page.getByText(PLACEHOLDER_TEXT)).toHaveCount(0);
244+
await expect(page.getByText(NOT_FOUND_TEXT)).toHaveCount(0);
245+
});
246+
}
247+
248+
test('sidebar → Dashboard returns home', async ({ page }) => {
249+
// Leave the dashboard first so the click is a real navigation.
250+
await page.getByRole('button', { name: 'Inventory' }).click();
251+
await expect(page).toHaveURL(/\/inventory(\/|$)/, { timeout: LAZY_TIMEOUT });
252+
253+
await page.getByRole('button', { name: 'Dashboard' }).click();
254+
await expect(page).toHaveURL(/\/$/);
255+
await expect(page.getByRole('heading', { name: 'Dashboard' })).toBeVisible();
256+
await expect(page.getByText(NOT_FOUND_TEXT)).toHaveCount(0);
257+
});
258+
});

frontend/packages/shared/src/index.ts

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,22 +26,38 @@
2626
* shared across all packages (e.g., route paths, feature flags).
2727
*/
2828

29-
// ── Route paths (single source of truth for navigation + router) ──────
29+
// ── Route paths (single source of truth for the sidebar) ──────────────
30+
//
31+
// navigation.tsx (the sidebar) reads every `to:` from this map. router.tsx
32+
// still declares its own `path` literals (it is NOT wired to this map), so
33+
// each value here SHOULD match the corresponding router path and, for
34+
// schema-driven plugins, that plugin's `PLUGIN_BASE_PATH`. Nothing enforces
35+
// that at compile time — the `sidebar-navigation` e2e spec is the safety net
36+
// that DETECTS drift (it cannot prevent it). When migrating a plugin, update
37+
// the router route and the entry here together (see SEP-1270). Convention:
38+
// • schema-driven plugins live under /plugins/<name> (checksums, mysql_backups, archives)
39+
// • domain-grouped backups keep /backups/<db> (mongodb, postgresql)
40+
// • cross-cutting tools stay at bare top-level paths (inventory, tasks, …)
41+
// `schema*` entries below are legacy /schema-change/* aliases that router.tsx
42+
// still routes; they have no sidebar consumer but are kept intentionally.
3043
export const ROUTES = {
3144
dashboard: '/',
3245
login: '/login',
3346
inventory: '/inventory',
3447
tasks: '/tasks',
3548
snippets: '/snippets',
49+
atw: '/atw',
3650
alertTemplates: '/alerts/templates',
3751
alertTroubleshooting: '/alerts/troubleshooting',
3852
schemaAlters: '/schema-change/alters',
3953
schemaChecksums: '/schema-change/checksums',
4054
schemaInventory: '/schema-change/inventory',
41-
backupsMysql: '/backups/mysql',
55+
checksums: '/plugins/checksums',
56+
mysqlBackups: '/plugins/mysql_backups',
4257
backupsMongodb: '/backups/mongodb',
4358
backupsPostgresql: '/backups/postgresql',
44-
archive: '/archive',
59+
archive: '/plugins/archives',
60+
dipper: '/dipper',
4561
reports: '/reports',
4662
settings: '/settings',
4763
} as const;

frontend/packages/shell/src/contexts/navigation.tsx

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ import BarChartIcon from '@mui/icons-material/BarChart';
3232
import SupportAgentIcon from '@mui/icons-material/SupportAgent';
3333
import ScienceIcon from '@mui/icons-material/Science';
3434
import { MySqlIcon, MongoIcon, PostgreSqlIcon } from '@percona/percona-ui';
35+
import { ROUTES } from '@sep/shared';
3536
import type { SvgIconComponent } from '@mui/icons-material';
3637
import type { SvgIconProps } from '@mui/material';
3738

@@ -44,38 +45,49 @@ export interface NavItem {
4445

4546
// Navigation matching SEP's plugin-based sidebar.
4647
// Backup sub-items use percona-ui's database-specific icons.
48+
//
49+
// URL convention (see SEP-1270): every `to:` here is sourced from the shared
50+
// `ROUTES` map. router.tsx keeps its own `path` literals and is not wired to
51+
// ROUTES, so when a plugin is migrated to React update BOTH the router route
52+
// and the matching ROUTES entry together — never hardcode a path string here.
53+
// Drift between the two is what made the sidebar point at PlaceholderPage /
54+
// NotFoundPage (the regression this ticket fixed for MySQL & Archive); the
55+
// `sidebar-navigation` e2e spec now guards against it. Paths follow three families:
56+
// • /plugins/<name> — schema-driven plugins (checksums, mysql_backups, archives)
57+
// • /backups/<db> — domain-grouped backup plugins (mongodb, postgresql)
58+
// • bare top-level — cross-cutting tools (inventory, tasks, snippets, atw, dipper)
4759
const defaultNavItems: NavItem[] = [
48-
{ title: 'Dashboard', icon: DashboardIcon, to: '/' },
49-
{ title: 'Inventory', icon: DnsIcon, to: '/inventory' },
50-
{ title: 'Tasks', icon: AssignmentIcon, to: '/tasks' },
51-
{ title: 'Snippets', icon: CodeIcon, to: '/snippets' },
52-
{ title: 'Collect Diagnostic Data', icon: SupportAgentIcon, to: '/atw' },
60+
{ title: 'Dashboard', icon: DashboardIcon, to: ROUTES.dashboard },
61+
{ title: 'Inventory', icon: DnsIcon, to: ROUTES.inventory },
62+
{ title: 'Tasks', icon: AssignmentIcon, to: ROUTES.tasks },
63+
{ title: 'Snippets', icon: CodeIcon, to: ROUTES.snippets },
64+
{ title: 'Collect Diagnostic Data', icon: SupportAgentIcon, to: ROUTES.atw },
5365
{
5466
title: 'Alerts',
5567
icon: NotificationsActiveIcon,
5668
children: [
57-
{ title: 'Templates', icon: DescriptionIcon, to: '/alerts/templates' },
58-
{ title: 'Troubleshooting', icon: TroubleshootIcon, to: '/alerts/troubleshooting' },
69+
{ title: 'Templates', icon: DescriptionIcon, to: ROUTES.alertTemplates },
70+
{ title: 'Troubleshooting', icon: TroubleshootIcon, to: ROUTES.alertTroubleshooting },
5971
],
6072
},
6173
{
6274
title: 'Schema Change',
6375
icon: StorageIcon,
64-
children: [{ title: 'Alters', icon: TableChartIcon, to: '/schema-change/alters' }],
76+
children: [{ title: 'Alters', icon: TableChartIcon, to: ROUTES.schemaAlters }],
6577
},
66-
{ title: 'Checksums', icon: CheckCircleIcon, to: '/plugins/checksums' },
78+
{ title: 'Checksums', icon: CheckCircleIcon, to: ROUTES.checksums },
6779
{
6880
title: 'Backups',
6981
icon: BackupIcon,
7082
children: [
71-
{ title: 'MySQL', icon: MySqlIcon, to: '/backups/mysql' },
72-
{ title: 'MongoDB', icon: MongoIcon, to: '/backups/mongodb' },
73-
{ title: 'PostgreSQL', icon: PostgreSqlIcon, to: '/backups/postgresql' },
83+
{ title: 'MySQL', icon: MySqlIcon, to: ROUTES.mysqlBackups },
84+
{ title: 'MongoDB', icon: MongoIcon, to: ROUTES.backupsMongodb },
85+
{ title: 'PostgreSQL', icon: PostgreSqlIcon, to: ROUTES.backupsPostgresql },
7486
],
7587
},
76-
{ title: 'Archive', icon: ArchiveIcon, to: '/archive' },
77-
{ title: 'Dipper Data Collection', icon: ScienceIcon, to: '/dipper' },
78-
{ title: 'Reports', icon: BarChartIcon, to: '/reports' },
88+
{ title: 'Archive', icon: ArchiveIcon, to: ROUTES.archive },
89+
{ title: 'Dipper Data Collection', icon: ScienceIcon, to: ROUTES.dipper },
90+
{ title: 'Reports', icon: BarChartIcon, to: ROUTES.reports },
7991
];
8092

8193
interface NavigationState {

frontend/packages/shell/src/router.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,9 @@ export const router = createBrowserRouter([
9797
{ path: 'schema-change/checksums/*', element: <ChecksumsPlugin /> },
9898
{ path: 'plugins/mysql_backups/*', element: <MysqlBackupsPlugin /> },
9999
{ path: 'schema-change/inventory/*', element: <InventoryPlugin /> },
100-
{ path: 'backups/mysql', element: <PlaceholderPage /> },
100+
// NOTE: MySQL Backups lives at /plugins/mysql_backups (above), matching
101+
// its PLUGIN_BASE_PATH. The old /backups/mysql placeholder route was
102+
// removed in SEP-1270 — the sidebar now points at the real plugin.
101103
{ path: 'backups/mongodb/*', element: <BackupMongoPlugin /> },
102104
{ path: 'backups/postgresql/*', element: <BackupPgPlugin /> },
103105
{ path: 'plugins/archives/*', element: <ArchivesPlugin /> },

0 commit comments

Comments
 (0)