docs: plan settings navigation performance fix

2026-07-30 22:06:28 +08:00
parent 3bbd227c58
commit af5abb4b82
1 changed files with 456 additions and 0 deletions
@@ -0,0 +1,456 @@
+# Settings Navigation And Stream Performance Implementation Plan
+
+> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
+
+**Goal:** Make top-level settings security-only, switch internal pages without document reloads, and let the browser observe an idle SSE connection immediately.
+
+**Architecture:** Keep the existing pathname-based `AppShell` and add one focused browser-navigation Hook that owns pathname state, eligible link interception, history traversal, focus, and scroll restoration. Keep the instance editor route intact while turning the legacy top-level instance-settings route into the existing redirect route shape. Preserve streaming proxy behavior and explicitly flush downstream headers as soon as upstream headers arrive.
+
+**Tech Stack:** React 19, TypeScript 5.9, Testing Library, Vitest/jsdom, Node HTTP, Chrome DevTools Protocol browser E2E
+
+## Global Constraints
+
+- Do not add a third-party router.
+- Top-level settings contains only password protection and HTTP/HTTPS security information.
+- `/settings/instances` resolves as a redirect to `/settings/system`.
+- `/settings/instances/:instanceId` remains available for instance editing from node details.
+- Eligible same-origin links use `history.pushState`; external, download, target, cross-origin, and modified-click links retain browser behavior.
+- Browser back and forward remain functional through `popstate`.
+- Page changes focus `#main-content` and scroll to the top.
+- `AppShell` remains mounted across top-navigation changes so the event subscription is not recreated.
+- SSE remains streaming and authenticated; do not replace it with polling or conceal genuine reconnect states.
+- Production code changes must follow an observed failing test.
+- Do not push the resulting commits unless the user explicitly requests it.
+
+---
+
+## File Structure
+
+- `apps/web/src/app-shell.tsx`: route ownership and the three-entry global navigation; no browser event ownership.
+- `apps/web/src/app-shell.integration.test.tsx`: route, navigation, security-only settings, and instance-editor regression coverage.
+- `apps/web/src/navigation/use-browser-pathname.ts`: browser pathname state and document-level navigation interception.
+- `apps/web/src/navigation/use-browser-pathname.test.tsx`: jsdom behavior contract for same-page navigation.
+- `apps/web/src/main.tsx`: thin browser root that supplies the Hook pathname to the persistent `AppShell`.
+- `apps/web/src/styles.css`: remove now-unused top-level settings sub-navigation styles.
+- `apps/api/src/canary-gateway.ts`: downstream proxy header forwarding.
+- `apps/api/src/canary-gateway.test.ts`: idle SSE header-flush regression.
+- `scripts/real-browser-e2e.mjs`: browser-level assertions for navigation persistence, request volume, focus, and live status.
+
+### Task 1: Security-Only Top-Level Settings Route
+
+**Files:**
+- Modify: `apps/web/src/app-shell.integration.test.tsx`
+- Modify: `apps/web/src/app-shell.tsx`
+- Modify: `apps/web/src/styles.css`
+
+**Interfaces:**
+- Consumes: existing `resolveRoute(input: string): ResolvedRoute` and `AppShellProps`.
+- Produces: `resolveRoute('/settings/instances')` with `{ kind: 'redirect', to: '/settings/system' }`; global settings link `href="/settings/system"`.
+
+- [ ] **Step 1: Write failing route and rendering tests**
+
+Add an explicit `resolveRoute` import and assertions to `app-shell.integration.test.tsx`:
+
+```tsx
+expect(resolveRoute('/settings/instances')).toMatchObject({
+  kind: 'redirect',
+  to: '/settings/system',
+});
+expect(resolveRoute('/settings/instances/bravo')).toMatchObject({
+  kind: 'settings-instance-detail',
+  params: { instanceId: 'bravo' },
+});
+```
+
+Update the global-workspace test so settings points directly to `/settings/system`, the system settings content renders, and there is no top-level settings sub-navigation:
+
+```tsx
+expect(within(navigation).getByRole('link', { name: '设置' })).toHaveAttribute(
+  'href',
+  '/settings/system',
+);
+rerender(<AppShell pathname="/settings/system" eventStreamClient={quietEventStreamClient} />);
+expect(screen.getByRole('heading', { name: '系统与安全' })).toBeTruthy();
+expect(screen.queryByRole('navigation', { name: '设置导航' })).toBeNull();
+```
+
+Render `/settings/instances` with a spying Fleet source and assert it renders security settings without loading Fleet:
+
+```tsx
+const load = vi.fn().mockResolvedValue(snapshot);
+render(
+  <AppShell
+    pathname="/settings/instances"
+    fleetDataSource={source(load)}
+    eventStreamClient={quietEventStreamClient}
+  />,
+);
+expect(screen.getByRole('heading', { name: '系统与安全' })).toBeTruthy();
+expect(load).not.toHaveBeenCalled();
+```
+
+- [ ] **Step 2: Run the focused test and verify failure**
+
+Run: `corepack pnpm exec vitest run apps/web/src/app-shell.integration.test.tsx`
+
+Expected: FAIL because settings still links to `/settings/instances`, renders the settings sub-navigation, and owns the Fleet-backed instance settings page.
+
+- [ ] **Step 3: Implement the minimal settings route change**
+
+In `resolveRoute`, remove `/settings/instances` from `staticRoutes` and return the redirect before dynamic instance-edit matching:
+
+```tsx
+if (pathname === '/settings/instances')
+  return { kind: 'redirect', pathname, to: '/settings/system' };
+```
+
+Change the global item and remove the obsolete page branch and import:
+
+```tsx
+{ section: 'settings', href: '/settings/system', label: '设置', icon: 'settings' }
+```
+
+Delete the conditional `<nav className="settings-navigation">` block. Remove both `.settings-navigation` rule groups from `styles.css`; they no longer have a consumer.
+
+- [ ] **Step 4: Run the focused test and verify success**
+
+Run: `corepack pnpm exec vitest run apps/web/src/app-shell.integration.test.tsx`
+
+Expected: PASS, including the preserved `/settings/instances/bravo` editor route.
+
+- [ ] **Step 5: Commit the settings slice**
+
+```powershell
+git add apps/web/src/app-shell.tsx apps/web/src/app-shell.integration.test.tsx apps/web/src/styles.css
+git commit -m "fix: simplify top-level settings"
+```
+
+### Task 2: Same-Page Browser Pathname Hook
+
+**Files:**
+- Create: `apps/web/src/navigation/use-browser-pathname.ts`
+- Create: `apps/web/src/navigation/use-browser-pathname.test.tsx`
+- Modify: `apps/web/src/main.tsx`
+
+**Interfaces:**
+- Consumes: browser `window.location`, `window.history`, `document` click events, and `popstate`.
+- Produces: `useBrowserPathname(): string`.
+
+- [ ] **Step 1: Write failing Hook tests**
+
+Create a jsdom test harness:
+
+```tsx
+// @vitest-environment jsdom
+import { cleanup, fireEvent, render, screen } from '@testing-library/react';
+import { afterEach, describe, expect, it, vi } from 'vitest';
+import { useBrowserPathname } from './use-browser-pathname.js';
+
+function Harness() {
+  const pathname = useBrowserPathname();
+  return (
+    <>
+      <a href="/automation">Automation</a>
+      <main id="main-content" tabIndex={-1}>{pathname}</main>
+    </>
+  );
+}
+
+afterEach(() => {
+  cleanup();
+  window.history.replaceState(null, '', '/');
+  vi.restoreAllMocks();
+});
+```
+
+Cover these exact contracts:
+
+```tsx
+it('pushes eligible same-origin links and updates pathname without reloading', () => {
+  window.history.replaceState(null, '', '/fleet');
+  const pushState = vi.spyOn(window.history, 'pushState');
+  const scrollTo = vi.spyOn(window, 'scrollTo').mockImplementation(() => undefined);
+  render(<Harness />);
+  fireEvent.click(screen.getByRole('link', { name: 'Automation' }));
+  expect(pushState).toHaveBeenCalledWith(null, '', '/automation');
+  expect(screen.getByRole('main')).toHaveTextContent('/automation');
+  expect(screen.getByRole('main')).toHaveFocus();
+  expect(scrollTo).toHaveBeenCalledWith({ top: 0, left: 0, behavior: 'auto' });
+});
+```
+
+Add a `popstate` test that replaces history with `/fleet`, dispatches `new PopStateEvent('popstate')`, and expects the rendered pathname to update. Add parameterized anchor tests proving no interception for `target="_blank"`, `download`, an external origin, hash-only navigation, `button: 1`, and each of `metaKey`, `ctrlKey`, `shiftKey`, and `altKey`.
+
+- [ ] **Step 2: Run Hook tests and verify failure**
+
+Run: `corepack pnpm exec vitest run apps/web/src/navigation/use-browser-pathname.test.tsx`
+
+Expected: FAIL because `use-browser-pathname.ts` does not exist.
+
+- [ ] **Step 3: Implement the minimal Hook**
+
+Implement a React Hook with this public shape:
+
+```tsx
+export function useBrowserPathname(): string
+```
+
+The effect must register one document `click` listener and one window `popstate` listener, and clean up both. The click listener must locate `event.target.closest('a[href]')`, reject prevented/non-primary/modified clicks, reject `download` and non-`_self` targets, parse with `new URL(anchor.href, window.location.href)`, require the current origin, reject same-path hash-only changes, then call:
+
+```tsx
+event.preventDefault();
+window.history.pushState(null, '', `${url.pathname}${url.search}${url.hash}`);
+setPathname(window.location.pathname);
+window.scrollTo({ top: 0, left: 0, behavior: 'auto' });
+document.querySelector<HTMLElement>('#main-content')?.focus({ preventScroll: true });
+```
+
+The `popstate` listener updates state from `window.location.pathname`, scrolls, and focuses using the same local helper.
+
+- [ ] **Step 4: Run Hook tests and verify success**
+
+Run: `corepack pnpm exec vitest run apps/web/src/navigation/use-browser-pathname.test.tsx`
+
+Expected: PASS for interception, history traversal, exclusions, focus, and scroll.
+
+- [ ] **Step 5: Keep the AppShell mounted in `main.tsx`**
+
+Add a thin component and pass its reactive pathname to the existing shell:
+
+```tsx
+function BrowserApp() {
+  const pathname = useBrowserPathname();
+  return (
+    <ConsoleAuthGate>
+      <AppShell pathname={pathname} />
+    </ConsoleAuthGate>
+  );
+}
+
+createRoot(root).render(
+  <StrictMode>
+    <BrowserApp />
+  </StrictMode>,
+);
+```
+
+This changes only `AppShell` props during navigation; it does not recreate the root or shell component.
+
+- [ ] **Step 6: Run all Web unit tests**
+
+Run: `corepack pnpm --filter @multi-simadmin/web test`
+
+Expected: PASS.
+
+- [ ] **Step 7: Commit the navigation slice**
+
+```powershell
+git add apps/web/src/navigation/use-browser-pathname.ts apps/web/src/navigation/use-browser-pathname.test.tsx apps/web/src/main.tsx
+git commit -m "feat: navigate workspaces without page reloads"
+```
+
+### Task 3: Flush Idle SSE Headers Through The Canary Gateway
+
+**Files:**
+- Modify: `apps/api/src/canary-gateway.test.ts`
+- Modify: `apps/api/src/canary-gateway.ts`
+
+**Interfaces:**
+- Consumes: upstream Node `IncomingMessage` headers and downstream `ServerResponse`.
+- Produces: downstream response headers observable before the first upstream body byte.
+
+- [ ] **Step 1: Write the failing idle-stream regression test**
+
+Add a test whose upstream sends and flushes headers but deliberately sends no body:
+
+```tsx
+it('flushes SSE response headers before the first upstream event', async () => {
+  const upstream = createServer((_request, response) => {
+    response.writeHead(200, {
+      'content-type': 'text/event-stream',
+      'cache-control': 'no-cache',
+    });
+    response.flushHeaders();
+  });
+  servers.push(upstream);
+  const gateway = await startGateway(await fixtureDist(), await listen(upstream));
+
+  const controller = new AbortController();
+  const timeout = setTimeout(() => controller.abort(), 500);
+  try {
+    const response = await fetch(`${gateway.origin}/api/v1/events`, {
+      signal: controller.signal,
+    });
+    expect(response.status).toBe(200);
+    expect(response.headers.get('content-type')).toBe('text/event-stream');
+    await response.body?.cancel();
+  } finally {
+    clearTimeout(timeout);
+  }
+});
+```
+
+- [ ] **Step 2: Run the gateway test and verify failure**
+
+Run: `corepack pnpm exec vitest run apps/api/src/canary-gateway.test.ts -t "flushes SSE response headers"`
+
+Expected: FAIL by abort/timeout because the gateway has not committed downstream headers.
+
+- [ ] **Step 3: Flush downstream headers after forwarding upstream headers**
+
+Immediately after the existing `response.writeHead(...)` in `proxyRequest`, add:
+
+```tsx
+response.flushHeaders();
+```
+
+Keep the existing `pipeline(upstreamResponse, response)` unchanged.
+
+- [ ] **Step 4: Run gateway tests and verify success**
+
+Run: `corepack pnpm exec vitest run apps/api/src/canary-gateway.test.ts`
+
+Expected: PASS for both the idle-header and multi-chunk streaming cases.
+
+- [ ] **Step 5: Commit the gateway slice**
+
+```powershell
+git add apps/api/src/canary-gateway.ts apps/api/src/canary-gateway.test.ts
+git commit -m "fix: flush proxied event stream headers"
+```
+
+### Task 4: Real Browser Navigation And Connection Regression
+
+**Files:**
+- Modify: `scripts/real-browser-e2e.mjs`
+
+**Interfaces:**
+- Consumes: built Web assets, isolated fixture server or `E2E_ORIGIN`, Chrome DevTools Protocol.
+- Produces: end-to-end proof that navigation is reload-free, does not recreate SSE, does not load Fleet from settings, and reaches the connected status.
+
+- [ ] **Step 1: Add instrumentation and failing browser assertions**
+
+Change the fixture event endpoint to return an open SSE response:
+
+```js
+if (pathname === '/api/v1/events') {
+  response.writeHead(200, {
+    'content-type': 'text/event-stream',
+    'cache-control': 'no-cache',
+  });
+  response.flushHeaders();
+  return;
+}
+```
+
+Record request pathnames in the isolated server and install a page-lifetime marker before navigating:
+
+```js
+await evaluate(cdp, `window.__navigationLifetime = crypto.randomUUID()`);
+const lifetime = await evaluate(cdp, `window.__navigationLifetime`);
+```
+
+After keyboard navigation from Fleet to settings, assert:
+
+```js
+assert.equal(await evaluate(cdp, `window.__navigationLifetime`), lifetime);
+assert.equal(await evaluate(cdp, `location.pathname`), '/settings/system');
+assert.equal(
+  await evaluate(cdp, `document.activeElement?.id`),
+  'main-content',
+  'Settings navigation must focus the main content',
+);
+```
+
+Use the recorded request counts before and after navigation to assert that `/api/v1/events` remains at one request and that settings navigation adds no `/api/v1/instances` or `/resources` request. Wait until `.connection-status [data-state="open"]` exists before asserting.
+
+- [ ] **Step 2: Build and run browser E2E to expose remaining failures**
+
+Run: `corepack pnpm run test:e2e:browser`
+
+Expected before all preceding tasks are applied: FAIL on the old settings path, lifetime marker, event request count, or open connection status. Expected after Tasks 1-3: PASS.
+
+- [ ] **Step 3: Update existing E2E route expectations and diagnostics**
+
+Replace both settings navigation expectations from `/settings/instances` and heading `实例` to `/settings/system` and the system/security heading. Update the final PASS line to name same-page navigation and live SSE status. Keep the existing responsive Fleet and Automation assertions unchanged.
+
+- [ ] **Step 4: Re-run browser E2E**
+
+Run: `corepack pnpm run test:e2e:browser`
+
+Expected: PASS in Chrome/Edge with one event subscription, no document reload, no Fleet requests caused by settings, focused main content, and connected stream status.
+
+- [ ] **Step 5: Commit the browser regression**
+
+```powershell
+git add scripts/real-browser-e2e.mjs
+git commit -m "test: cover persistent browser navigation"
+```
+
+### Task 5: Full Verification And Local Runtime
+
+**Files:**
+- Verify only; modify a file only if a failing check demonstrates a defect in the scoped implementation.
+
+**Interfaces:**
+- Consumes: all prior task outputs.
+- Produces: a verified release candidate at `http://127.0.0.1:8789/`.
+
+- [ ] **Step 1: Run unit and integration tests**
+
+Run: `corepack pnpm test`
+
+Expected: PASS.
+
+- [ ] **Step 2: Run static checks**
+
+Run: `corepack pnpm typecheck`
+
+Expected: PASS.
+
+Run: `corepack pnpm lint`
+
+Expected: PASS.
+
+Run: `corepack pnpm format:check`
+
+Expected: PASS.
+
+- [ ] **Step 3: Run the production Web build and real browser suite**
+
+Run: `corepack pnpm --filter @multi-simadmin/web build`
+
+Expected: PASS and emit `apps/web/dist`.
+
+Run: `corepack pnpm run test:e2e:browser`
+
+Expected: PASS.
+
+- [ ] **Step 4: Restart the local production services using the repository's existing runtime commands**
+
+First inspect the current listeners and repository runtime documentation; stop only the exact API/gateway processes belonging to this workspace. Start the API on `127.0.0.1:8790` and the gateway on `127.0.0.1:8789` with the existing configured environment and hidden background windows.
+
+- [ ] **Step 5: Verify the live gateway**
+
+Run:
+
+```powershell
+curl.exe -fsS http://127.0.0.1:8789/readyz
+curl.exe -sS -N --max-time 2 -D - http://127.0.0.1:8789/api/v1/events
+```
+
+Expected: readiness succeeds; the event request prints `HTTP/1.1 200 OK` and `content-type: text/event-stream` before curl times out waiting for an event body.
+
+- [ ] **Step 6: Inspect final Git state**
+
+Run: `git status --short --branch`
+
+Expected: branch contains only the planned commits and is ahead of `origin/main`; do not push.
+
+---
+
+## Self-Review
+
+- Spec coverage: Tasks 1-5 cover security-only settings, legacy redirect semantics, retained instance editing, persistent same-page navigation, link exclusions, history traversal, focus/scroll behavior, idle SSE header delivery, genuine stream state, request-volume regression, and local `8789` verification.
+- Placeholder scan: no deferred implementation or unspecified error-handling steps remain.
+- Type consistency: `useBrowserPathname(): string`, existing `ResolvedRoute`, and existing `EventStreamClient` contracts are used consistently across tasks.