From 187d504ab72a013c53556bf97e052141d0756fe1 Mon Sep 17 00:00:00 2001
From: Csaba Tuncsik <csaba@n8n.io>
Date: Fri, 4 Sep 2026 12:08:24 +0000
Subject: [PATCH] chore: Rewrite the cubic Frontend review agent rules (#37458)

---
 .../frontend/component-wrappers.md            | 21 ++++++++
 .../review-rules/frontend/design-system.md    | 48 ++++++++++++++-----
 .agents/review-rules/frontend/i18n.md         | 18 +++++++
 .../review-rules/frontend/vue-correctness.md  | 43 +++++++++++++++++
 .../frontend/workflow-document-store.md       | 37 --------------
 .agents/review-rules/testing/coverage.md      |  9 +++-
 cubic.yaml                                    | 14 ++++--
 7 files changed, 136 insertions(+), 54 deletions(-)
 create mode 100644 .agents/review-rules/frontend/component-wrappers.md
 create mode 100644 .agents/review-rules/frontend/i18n.md
 create mode 100644 .agents/review-rules/frontend/vue-correctness.md
 delete mode 100644 .agents/review-rules/frontend/workflow-document-store.md

diff --git a/.agents/review-rules/frontend/component-wrappers.md b/.agents/review-rules/frontend/component-wrappers.md
new file mode 100644
index 00000000000..7bd69cc7e15
--- /dev/null
+++ b/.agents/review-rules/frontend/component-wrappers.md
@@ -0,0 +1,21 @@
+# Wrapping a third-party component
+
+Applies to: `packages/frontend`.
+
+`@n8n/design-system` wraps reka-ui, Element Plus, TipTap and CodeMirror. The
+recurring defect is assuming the wrapper passes something through, or that a
+library default is what you want.
+
+- **Attributes are forwarded only if the wrapper forwards them.** `N8nSelect`
+  binds just `$props` and `on*` listeners to `ElSelect`, so an `aria-label`,
+  `data-test-id` or native attribute set on it lands on the outer `<div>` and
+  never reaches the control. Check the wrapper's `v-bind`, and prefer its own
+  prop where one exists.
+- **Library defaults are behaviour.** A dialog focus trap focusing the first
+  tabbable child opens a filterable select on mount. Flag a handler
+  suppressing a library behaviour unconditionally when one state needs it,
+  e.g. a blanket `@interact-outside` `preventDefault`.
+- **Vendor class names are contracts.** A `:global()` selector against one
+  stops matching silently when the library renames it.
+- An accessible name computing to empty for an icon-only control, or a
+  `<button>` with no explicit `type`, which is `submit` inside a form.
diff --git a/.agents/review-rules/frontend/design-system.md b/.agents/review-rules/frontend/design-system.md
index 469931dc89d..c9070b200e3 100644
--- a/.agents/review-rules/frontend/design-system.md
+++ b/.agents/review-rules/frontend/design-system.md
@@ -1,16 +1,42 @@
-# Design system enforcement
+# Design system
 
 Applies to: `packages/frontend`.
 
-The design system skill linked alongside this file is the source of truth for
-which token to reach for. This file sets the enforcement level.
+Stylelint checks the *grammar* of a custom-property name, at warning severity.
+Nothing checks that a token exists or is the right one, and `var(--gone)`
+resolves to nothing rather than failing. Wrong tokens are the most common
+shipped frontend regression in this repo.
 
-Stylelint validates CSS custom-property *names* but never their values, so
-nothing catches a hard-coded one. Never comment on the naming itself — that
-half already fails the build.
+Three layers, narrowest first; reaching past the narrowest that fits is the
+defect:
 
-- Strong warning: hard-coded visual values (px, rem, hex colours, durations)
-  where a token exists; legacy token usage; deprecated style or component
-  surfaces.
-- Soft warning: token-to-token substitutions. Ask for intent rather than
-  asserting a regression — a deliberate change looks identical to a mistake.
+1. component-scoped: `--button--*`, `--callout--*`, `--input--*`, `--tag--*`,
+   `--node--*`, `--canvas-node--*`, `--sticky--*`
+2. semantic: `--background--*`, `--text-color--*`, `--border-color`
+3. primitives, in px: `--spacing--` 5xs 2, 4xs 4, 3xs 6, 2xs 8, xs 12, sm 16,
+   md 20, lg 24, xl 32, 2xl 48, 3xl 64, 4xl 128, 5xl 256; `--radius--` 4xs 2,
+   3xs 4, 2xs 6, xs 8, sm 12, md 16, lg 20, xl 24, 2xl 32, `--radius--full`
+
+Flag, strongest first:
+
+- A `var()` naming a token that no longer exists, e.g. a legacy `--color--*`
+  kept through a refactor. It renders as nothing, silently.
+- A hard-coded px, rem, hex, `rgb()`, ms or `z-index` literal where a token
+  covers it. Name the token.
+- A generic token where a component-scoped one exists, e.g. a `--color--*`
+  background on an input that has `--input--color--background`.
+- A px target stated in the diff or a comment that the chosen token misses.
+  The scales share suffixes at different values: `--radius--xs` is 8px,
+  `--spacing--xs` is 12px. "Design calls for 8px" beside `--radius--lg` (20px)
+  is a real defect.
+
+Soft warning on a plain token swap: ask the intent, since a deliberate change
+and a mistake look identical.
+
+Flag a hand-rolled control duplicating an `N8n*` component (icon button,
+tooltip, dialog, select), which arrives missing hover, focus and disabled
+states, and a NEW direct `element-plus` or `reka-ui` import in `editor-ui`.
+No lint rule covers those and many files predate the convention, so judge
+only the import being added.
+
+Do NOT comment on Storybook story titles, args, or doc cross-references.
diff --git a/.agents/review-rules/frontend/i18n.md b/.agents/review-rules/frontend/i18n.md
new file mode 100644
index 00000000000..d9555a1c45b
--- /dev/null
+++ b/.agents/review-rules/frontend/i18n.md
@@ -0,0 +1,18 @@
+# i18n
+
+Applies to: `packages/frontend`.
+
+`baseText(key, { interpolate })` takes an untyped record and vue-i18n leaves
+an unmatched placeholder in the output. Nothing checks those names against
+`en.json`.
+
+- An `interpolate` key not matching the `{placeholder}` in that string,
+  including a case difference such as `docUrl` against `{docURL}`. The user
+  sees the raw placeholder or nothing.
+- `i18n.baseText` passed as a bare function reference. It is a class method
+  reading `this`, so detaching it returns the key instead of the translation.
+  Pass the `i18n` object.
+- Punctuation, separators or brackets hardcoded around a translated string in
+  a template. Composition differs per locale; put them in the string.
+Do NOT flag a missing non-`en` translation, or wording itself unless it
+contradicts copy the same PR adds.
diff --git a/.agents/review-rules/frontend/vue-correctness.md b/.agents/review-rules/frontend/vue-correctness.md
new file mode 100644
index 00000000000..9dafe8f1587
--- /dev/null
+++ b/.agents/review-rules/frontend/vue-correctness.md
@@ -0,0 +1,43 @@
+# Vue correctness
+
+Applies to: `packages/frontend`.
+
+These fail silently: `vue-tsc` and ESLint pass, the app runs, the value or the
+DOM is wrong.
+
+## List identity
+
+The editor patches long-lived lists in place, so `:key` decides whether Vue
+reuses the wrong row. Flag an index `:key` on a list that can be reordered,
+filtered or shortened, where revealed fields, inputs and refs then follow the
+position instead of the item; a key omitting part of what identifies the
+instance, e.g. a session key without the execution id, so switching
+executions reuses the old component's state; and a key regenerated every
+render. Index keys are fine on an append-only list.
+
+## Reactivity
+
+- A `ref` or `computed` read without `.value` outside a template. Templates
+  unwrap, script does not, so you hold the wrapper: `activeNode?.type` reads
+  `undefined` instead of the type, without throwing.
+- A reactive object spread, cloned or deep-merged. `{ ...store }`,
+  `structuredClone` and recursive merge helpers detach it, and a merge into a
+  shared defaults object leaks one call into the next.
+- An `injectStrict` helper such as `injectNDVStore` called outside `setup()`.
+
+## State written after an await
+
+Navigation swaps the active workflow document, and dialogs, threads and NDV
+panels unmount mid-request. Flag a continuation writing to a store,
+`localStorage` or the route without re-checking what it captured before the
+`await`: navigating first and persisting afterwards what the destination was
+meant to read, clearing a selection the user has since changed, toasting for a
+closed dialog. Ask for a revalidated reference or a request sequence number,
+never a lock.
+
+## Payload shape
+
+Backend responses and `/rest/settings` omit optional sections entirely. Flag a
+new getter or computed walking more than one level into a response without
+optional chaining, e.g. `settings.value.license.planName`. It throws during
+boot and takes the view with it.
diff --git a/.agents/review-rules/frontend/workflow-document-store.md b/.agents/review-rules/frontend/workflow-document-store.md
deleted file mode 100644
index fdafc6a993c..00000000000
--- a/.agents/review-rules/frontend/workflow-document-store.md
+++ /dev/null
@@ -1,37 +0,0 @@
-# Migrated workflow fields must use workflowDocumentStore
-
-Applies to: `packages/frontend`.
-
-Workflow state is actively migrating from `workflowsStore`
-(`packages/frontend/editor-ui/src/app/stores/workflows.store.ts`) to
-`workflowDocumentStore`
-(`packages/frontend/editor-ui/src/app/stores/workflowDocument.store.ts`).
-
-`workflowDocumentStore` is the single source of truth for migrated fields. New
-code that reads or writes one of them must go through it, not through
-`workflowsStore.workflow.*` or an equivalent accessor.
-
-Already-migrated fields — do NOT access these via `workflowsStore`:
-
-`active`, `activeVersion`, `activeVersionId`, `checksum`, `createdAt`,
-`homeProject`, `meta`, `pinData`, `settings`, `tags`, `updatedAt`
-
-Flag when NEW code does any of:
-
-1. Reads a migrated field from `workflowsStore.workflow.<field>`
-2. Reads one from a destructured `workflow` ref originating in `workflowsStore`
-3. Writes one through `workflowsStore`, e.g. `workflowsStore.workflow.active = true`
-4. Calls a `workflowsStore` setter/action that only mutates a migrated field,
-   when `workflowDocumentStore` already exposes an equivalent method
-
-Do NOT flag:
-
-- Non-migrated fields, e.g. `workflowsStore.workflow.nodes`, `.connections`
-- Code inside `workflowDocument.store.ts` or its sub-modules — those *are* the
-  source of truth
-- Code inside `workflows.store.ts` itself — the migration is still in progress there
-
-Use `useWorkflowDocumentStore(createWorkflowDocumentId(workflowId))` from
-`@/app/stores/workflowDocument.store.ts`.
-
-This is an active migration; the field list will grow.
diff --git a/.agents/review-rules/testing/coverage.md b/.agents/review-rules/testing/coverage.md
index 21715dfecf9..8dd0a08e5d5 100644
--- a/.agents/review-rules/testing/coverage.md
+++ b/.agents/review-rules/testing/coverage.md
@@ -6,9 +6,16 @@ Flag new behaviour that ships with no test — a service method, controller
 route, node operation, store action, or composable. Coverage does not need to
 be complete; core functionality and the critical path do.
 
+Also flag a new or edited test whose assertion cannot tell the fixed behaviour
+from the broken one, since the suite going green is then the only signal
+anyone gets: a name stating the opposite of what is asserted, an assertion
+that holds whether or not the invariant it names does, or a case duplicating a
+sibling on the same path but for a mock the code never reads.
+
 Do NOT flag:
 
 - Exports, types, configuration objects, metadata, version files
 - A percentage — the number is not the bar, the critical path is
 - Edge cases a human reviewer is better placed to ask for
-- A skipped test — the `no-skipped-tests` ESLint rule already fails the build
+- A skipped test — `no-skipped-tests` already fails the build
+- A change breaking an existing test elsewhere; the suite runs on every PR
diff --git a/cubic.yaml b/cubic.yaml
index d7da070ad20..28d626ce4cb 100644
--- a/cubic.yaml
+++ b/cubic.yaml
@@ -173,9 +173,10 @@ reviews:
         plus the entity it touches and the queries that will use any new column.
     - name: Frontend
       file_paths:
-        - .agents/skills/design-system/SKILL.md
         - .agents/review-rules/frontend/design-system.md
-        - .agents/review-rules/frontend/workflow-document-store.md
+        - .agents/review-rules/frontend/vue-correctness.md
+        - .agents/review-rules/frontend/component-wrappers.md
+        - .agents/review-rules/frontend/i18n.md
         - .agents/review-rules/testing/coverage.md
       include:
         - packages/frontend/**
@@ -183,10 +184,13 @@ reviews:
         - '**/__tests__/**'
         - '**/*.stories.ts'
         - '**/*.spec.ts'
+        - '**/*.mdx'
       description: |-
-        Enforce n8n's frontend conventions using the linked rules. The design
-        system skill is the source of truth for which token to reach for; the
-        rule files that follow set the enforcement level.
+        Review n8n's frontend packages against the linked rules, drawn from
+        what actually shipped broken here. The editor is a long-lived SPA:
+        one component tree is reused across workflows, stores outlive the
+        views reading them, and navigation swaps the active workflow document
+        mid-request. Reason about what renders, not how the code reads.
     - name: QA & DX
       file_paths:
         - .agents/review-rules/qa-dx/docker-native-modules.md