From e51f686e5cbddf591fdbf5777fd334cc99ffb445 Mon Sep 17 00:00:00 2001 From: Sergey Kozyrenko Date: Wed, 22 Jul 2026 18:16:04 +0700 Subject: [PATCH] refactor: drop the header-order comment and the rest of the narration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment on AppHeaderActions claimed the component pins itself to the right. It does not — the sibling content block takes `flex-1` and pushes it there — and the rule it stated was an instruction to call sites, not a fact about the twelve characters below it. The convention now lives in docs/list_detail_pages.md, next to the detail-page recipe that needs it. Two more went the same way, by making the code carry the point instead: the knowledge header's two ids are now `documentId` and `routeKnowledgeId`, so there is nothing to warn about, and the separator that doubles up is gated on a named `hasViewRow`. In the pager spec the injected delay is held by an assertion that counts the samples taken before the sibling appears — removing the delay now fails the test instead of quietly emptying it. Across the e2e specs, six comments that only explained why a test exists are gone and five more are down to one line. Co-Authored-By: Claude Opus 4.8 --- frontend/docs/list_detail_pages.md | 16 ++++++++++++++++ frontend/e2e/mocks/cassettes/settings-prompts.ts | 8 ++------ frontend/e2e/mocks/cassettes/templates.ts | 7 +------ frontend/e2e/route-manifest.unit.test.ts | 6 +----- frontend/e2e/specs/cross/a11y.spec.ts | 3 +-- frontend/e2e/specs/cross/palette.spec.ts | 6 ------ frontend/e2e/specs/crud/template-detail.spec.ts | 2 -- frontend/e2e/specs/flows/pager.spec.ts | 6 ++++-- frontend/e2e/specs/real/stand-smoke.spec.ts | 4 +--- .../e2e/specs/settings/prompt-detail.spec.ts | 6 ------ .../src/components/layouts/app/app-header.tsx | 2 -- .../src/features/knowledges/knowledge-header.tsx | 16 +++++++--------- 12 files changed, 33 insertions(+), 49 deletions(-) diff --git a/frontend/docs/list_detail_pages.md b/frontend/docs/list_detail_pages.md index b0054960..6002e095 100644 --- a/frontend/docs/list_detail_pages.md +++ b/frontend/docs/list_detail_pages.md @@ -568,6 +568,22 @@ const entityNav = useEntityDetailNavigation(isNew ? null : entityId); // null wh // both reading `controller={entityNav}`. ``` +#### Ordering the header actions + +`` takes the remaining width (`flex-1`), so `` ends up +against the right edge and grows leftward. Two rules follow: + +1. **Controls that appear on a data condition go first** in the children. Everything after + them keeps its position when they arrive late — the flow header's Report button loads with + the task list, and the pager beside it must not move under a cursor that is clicking Next. +2. **Controls that are always meaningful for the route render always**, taking `disabled` from + an explicit loading flag rather than being unmounted while the entity is null. Unmounting + collapses the cluster for the length of every fetch, which costs a step per pager click. + +The flow header is the reference: `Report · favourite · pager · actions menu`. A page whose +entity can be genuinely absent (a not-found card) is the exception — it renders no actions at +all, because none of them are meaningful there. + ## Design rationale ### Why URL > storage diff --git a/frontend/e2e/mocks/cassettes/settings-prompts.ts b/frontend/e2e/mocks/cassettes/settings-prompts.ts index 6cf0f249..b75b94b4 100644 --- a/frontend/e2e/mocks/cassettes/settings-prompts.ts +++ b/frontend/e2e/mocks/cassettes/settings-prompts.ts @@ -19,12 +19,8 @@ const agentPrompt = (system: PromptType) => entity('AgentPrompt', { system: prom const agentPromptPair = (system: PromptType, human: PromptType) => entity('AgentPrompts', { human: prompt(human), system: prompt(system) }); -/** - * A realistic Go `text/template` for the prompt detail route: the backend parses these with - * text/template, so the `{{.Var}}` atoms and the fenced command must survive the editor's - * parse/serialize round-trip. Deliberately has no list-nested fence — that is a separate, - * already-tracked editor defect and would make this spec assert a known bug. - */ +/** No list-nested fence on purpose: that editor defect is tracked separately, and including it + * here would make the spec assert a known bug. */ export const RICH_PROMPT_TEMPLATE = [ '# Pentester', '', diff --git a/frontend/e2e/mocks/cassettes/templates.ts b/frontend/e2e/mocks/cassettes/templates.ts index d8cfe535..95e148bb 100644 --- a/frontend/e2e/mocks/cassettes/templates.ts +++ b/frontend/e2e/mocks/cassettes/templates.ts @@ -23,12 +23,7 @@ export const TEMPLATE_SEED = makeTemplate('11', 'E2E Seed Template', 'Scan the t const flowTemplates: ResultOf = { flowTemplates: [TEMPLATE_SEED] }; -/** - * Non-trivial body for the template detail route, whose editor loads it from the server. - * Carries the `{{PLACEHOLDER}}` atoms a flow template is built from, so a parse/serialize - * round-trip that drops them fails the spec. No list-nested fence — that is a separate, - * already-tracked editor defect. - */ +/** No list-nested fence on purpose: that editor defect is tracked separately. */ export const RICH_TEMPLATE_TEXT = [ '# Recon', '', diff --git a/frontend/e2e/route-manifest.unit.test.ts b/frontend/e2e/route-manifest.unit.test.ts index 2812b9da..9823a70d 100644 --- a/frontend/e2e/route-manifest.unit.test.ts +++ b/frontend/e2e/route-manifest.unit.test.ts @@ -20,11 +20,7 @@ const EXCLUDED: Record = { '/templates/new': 'create-mode variant of the template detail page', }; -/** - * Dynamic route builders are functions, so the static walk below cannot see them — a new detail - * route could otherwise stay outside every sweep with nothing failing. Each builder must say where - * it is covered, or why it is not; the test asserts this list matches the builders that exist. - */ +/** Route builders are functions, so the static walk below cannot see them. */ const DYNAMIC_ROUTES: Record = { flow: 'manifest entry (routes.flow("5"))', flowReport: 'not swept: needs a finished-flow report cassette', diff --git a/frontend/e2e/specs/cross/a11y.spec.ts b/frontend/e2e/specs/cross/a11y.spec.ts index 917fffa0..9e9fdb77 100644 --- a/frontend/e2e/specs/cross/a11y.spec.ts +++ b/frontend/e2e/specs/cross/a11y.spec.ts @@ -30,8 +30,7 @@ for (const theme of THEMES) { test.describe('settings providers (populated)', () => { test.use({ cassette: populatedSettingsProvidersCassette() }); - // Not a duplicate of the manifest scan: that one sweeps the empty state, which - // renders no provider cards at all. + // The manifest scan sweeps this route empty, so it renders no provider cards. test('provider cards have no axe violations', async ({ page }) => { await page.goto('/settings/providers'); await expect(page.getByText('My Custom Endpoint')).toBeVisible(); diff --git a/frontend/e2e/specs/cross/palette.spec.ts b/frontend/e2e/specs/cross/palette.spec.ts index 06158629..c4ed6574 100644 --- a/frontend/e2e/specs/cross/palette.spec.ts +++ b/frontend/e2e/specs/cross/palette.spec.ts @@ -6,7 +6,6 @@ import { buttonVariants } from '@/components/ui/button'; import { expect, test } from '../../fixtures/test.ts'; import { ROUTE_MANIFEST } from '../../routes.ts'; -// Keyed off the unions so a new variant joins the sanctioned set without a manual edit. const BADGE_VARIANTS = Object.keys({ blue: true, default: true, @@ -43,11 +42,6 @@ const ACCEPTED: Record = { '/resources': ['button: hover:text-blue-400'], }; -/** - * The contrast gate probes cva output, so a colour written straight into a page — the shape of the - * one badge defect that shipped — is outside it by construction. This walks the rendered DOM - * instead: every badge and button must draw its colour from the variant set or from a token. - */ test.describe('palette compliance', { tag: '@cross' }, () => { for (const entry of ROUTE_MANIFEST) { test.describe(entry.path, () => { diff --git a/frontend/e2e/specs/crud/template-detail.spec.ts b/frontend/e2e/specs/crud/template-detail.spec.ts index 0286f58d..39ec9403 100644 --- a/frontend/e2e/specs/crud/template-detail.spec.ts +++ b/frontend/e2e/specs/crud/template-detail.spec.ts @@ -4,8 +4,6 @@ import { expect, test } from '../../fixtures/test.ts'; import { expectCleanPage } from '../../helpers/errors.ts'; import { RICH_TEMPLATE_TEXT, TEMPLATE_DETAIL, templateDetailCassette } from '../../mocks/cassettes/templates.ts'; -// The other route whose editor loads server content: the templates list spec only exercises -// create mode, so nothing covered the load path in the production bundle this tier runs. test.describe('template detail', { tag: '@coverage' }, () => { test.use({ cassette: templateDetailCassette() }); diff --git a/frontend/e2e/specs/flows/pager.spec.ts b/frontend/e2e/specs/flows/pager.spec.ts index 114232eb..e32c9666 100644 --- a/frontend/e2e/specs/flows/pager.spec.ts +++ b/frontend/e2e/specs/flows/pager.spec.ts @@ -29,8 +29,6 @@ test.describe('flow pager', { tag: ['@flows', '@smoke'] }, () => { await page.goto('/flows/5'); await expect(header.getByText('E2E Alpha')).toBeVisible(); - // Without the delay the cassette answers before the first sample and the loop below - // passes against an unmounting pager — measured. await page.route('**/graphql', async (route) => { await new Promise((resolve) => setTimeout(resolve, 300)); await route.fallback(); @@ -44,6 +42,7 @@ test.describe('flow pager', { tag: ['@flows', '@smoke'] }, () => { for (let index = 0; index < 10; index += 1) { taken.push({ hasPager: !!document.querySelector('header button[aria-label="Next"]'), + isSiblingShown: document.querySelector('header')?.textContent?.includes('E2E Beta') ?? false, path: window.location.pathname, }); await new Promise((resolve) => setTimeout(resolve, 30)); @@ -55,6 +54,9 @@ test.describe('flow pager', { tag: ['@flows', '@smoke'] }, () => { await expect(page).toHaveURL(/\/flows\/6$/); await expect(header.getByText('E2E Beta')).toBeVisible(); + // Guards the delay above as much as the pager: without it the sibling lands inside the + // first sample and the loop measures nothing. + expect(samples.filter((sample) => !sample.isSiblingShown).length).toBeGreaterThan(2); expect(samples.every((sample) => sample.hasPager)).toBe(true); expect(samples.map((sample) => sample.path)).not.toContain('/flows'); diff --git a/frontend/e2e/specs/real/stand-smoke.spec.ts b/frontend/e2e/specs/real/stand-smoke.spec.ts index b0a01114..e806e6f8 100644 --- a/frontend/e2e/specs/real/stand-smoke.spec.ts +++ b/frontend/e2e/specs/real/stand-smoke.spec.ts @@ -28,9 +28,7 @@ test.describe('stand smoke', { tag: '@stand' }, () => { // span tag matters: NavLink puts aria-current="page" on the active // settings-sidebar at runtime, the breadcrumb title is a span. await expect(page.locator('span[aria-current="page"]')).toHaveText(title); - // The only assertion here a backend erroring on every request fails: each page - // returns early into its ErrorState, rendering neither branch. Both branches are - // accepted because a stand may legitimately hold no rows. + // Either branch: a query error renders neither, and a stand may hold no rows. await expect(page.locator('[data-slot="table"]').first().or(page.getByText(emptyTitle))).toBeVisible(); expect(pageErrors, `uncaught errors on ${path}`).toEqual([]); }); diff --git a/frontend/e2e/specs/settings/prompt-detail.spec.ts b/frontend/e2e/specs/settings/prompt-detail.spec.ts index 09cf50ba..c61f6b1b 100644 --- a/frontend/e2e/specs/settings/prompt-detail.spec.ts +++ b/frontend/e2e/specs/settings/prompt-detail.spec.ts @@ -8,9 +8,6 @@ import { RICH_PROMPT_TEMPLATE, } from '../../mocks/cassettes/settings-prompts.ts'; -// The route the editor loads a real Go text/template into. Covered by nothing else: the prompts -// list spec expands the row in place, which renders a 
, never the editor — and the editor's
-// one shipped crash reproduced only in a production build, which is what this tier runs.
 test.describe('settings prompt detail', { tag: '@coverage' }, () => {
     test.use({ cassette: promptDetailCassette() });
 
@@ -29,7 +26,6 @@ test.describe('settings prompt detail', { tag: '@coverage' }, () => {
         const editor = page.getByRole('textbox', { name: EDITOR });
 
         await expect(editor).toBeVisible();
-        // The rich editor parsed the markdown rather than showing source.
         await expect(editor.getByRole('heading', { name: 'Pentester' })).toBeVisible();
         await expect(editor.getByText('nmap -sV {{.Target}}')).toBeVisible();
 
@@ -46,8 +42,6 @@ test.describe('settings prompt detail', { tag: '@coverage' }, () => {
 
         await expect(editor.getByRole('heading', { name: 'Pentester' })).toBeVisible();
 
-        // Typing makes the rich editor emit its own serialization — the half a load-only
-        // assertion cannot see, and where a parse/serialize defect would drop content.
         await editor.click();
         await page.keyboard.press('ControlOrMeta+End');
         await editor.pressSequentially(' E2E-MARK');
diff --git a/frontend/src/components/layouts/app/app-header.tsx b/frontend/src/components/layouts/app/app-header.tsx
index 3ab6e4fb..3b37b2a2 100644
--- a/frontend/src/components/layouts/app/app-header.tsx
+++ b/frontend/src/components/layouts/app/app-header.tsx
@@ -55,8 +55,6 @@ export function AppHeaderAction({
     );
 }
 
-// Right-pinned: it grows leftward, so a control that comes and goes only leaves the ones
-// before it in place. Put those first.
 export function AppHeaderActions({ children, className }: { children?: ReactNode; className?: string }) {
     return 
{children}
;
 }
diff --git a/frontend/src/features/knowledges/knowledge-header.tsx b/frontend/src/features/knowledges/knowledge-header.tsx
index 00f01b64..6ab4a9d7 100644
--- a/frontend/src/features/knowledges/knowledge-header.tsx
+++ b/frontend/src/features/knowledges/knowledge-header.tsx
@@ -87,8 +87,7 @@ export function KnowledgeHeader({
     const [isDeleting, setIsDeleting] = useState(false);
     const [isDeleteDialogOpen, setIsDeleteDialogOpen] = useState(false);
 
-    const knowledgeId = knowledge?.id ?? null;
-    // The route's id, not `knowledgeId` above: the pager must work while the document loads.
+    const documentId = knowledge?.id ?? null;
     const knowledgeNav = useKnowledgeDetailNavigation(isNew ? null : (routeKnowledgeId ?? null));
 
     // Title source-of-truth is the server-side `question`. We intentionally do
@@ -99,6 +98,7 @@ export function KnowledgeHeader({
     const hasKnowledge = !!knowledge;
     const isEntityPending = !hasKnowledge;
     const hasAnonymizeRow = isMobile && canAnonymize;
+    const hasViewRow = !!onModeChange;
     const hasEntityRows = !isNew;
     const hasNavRow = isMobile && !isNew;
 
@@ -108,7 +108,7 @@ export function KnowledgeHeader({
         isEditing: isEditingTitle,
         startEdit: handleRenameStart,
         stopEdit: handleRenameCancel,
-    } = useInlineEdit({ resetKey: knowledgeId });
+    } = useInlineEdit({ resetKey: documentId });
 
     const handleRenameSave = useCallback(async () => {
         const newQuestion = editingInputRef.current?.value.trim();
@@ -139,14 +139,14 @@ export function KnowledgeHeader({
     }, [editingInputRef, handleRenameCancel, knowledge, renameKnowledge]);
 
     const handleDelete = useCallback(async () => {
-        if (!knowledgeId) {
+        if (!documentId) {
             return;
         }
 
         setIsDeleting(true);
 
         try {
-            await deleteKnowledge(knowledgeId);
+            await deleteKnowledge(documentId);
             onBeforeNavigateAway?.();
             navigate(routes.knowledges, { replace: true });
         } catch {
@@ -154,7 +154,7 @@ export function KnowledgeHeader({
         } finally {
             setIsDeleting(false);
         }
-    }, [knowledgeId, deleteKnowledge, navigate, onBeforeNavigateAway]);
+    }, [documentId, deleteKnowledge, navigate, onBeforeNavigateAway]);
 
     return (
         <>
@@ -307,9 +307,7 @@ export function KnowledgeHeader({
                             ) : null}
                             {hasEntityRows && (
                                 <>
-                                    {/* Unconditional it doubles up: callers without a mode toggle
-                                        render nothing between the two groups. */}
-                                    {onModeChange ?  : null}
+                                    {hasViewRow && }
                                      setIsDeleteDialogOpen(true)}