diff --git a/brain/knowledge/decisions/000035-platform-admin-is-fifteen-pages-derived-from-one-registry.md b/brain/knowledge/decisions/000035-platform-admin-is-fifteen-pages-derived-from-one-registry.md new file mode 100644 index 000000000000..a36376c7cd0a --- /dev/null +++ b/brain/knowledge/decisions/000035-platform-admin-is-fifteen-pages-derived-from-one-registry.md @@ -0,0 +1,443 @@ +--- +status: accepted +--- + +# Platform admin is fifteen pages derived from one registry + +## Decision + +The platform admin nav collapses from **22 items in 5 labelled groups to 15 items in 3 unlabelled blocks**, +and every page, route and gate derives from **one declarative registry** rather than the three unconnected +places that held them. Six of the fifteen pages have sections: Users & access, Connections, Security, +AI Center & MCP, Pieces, and Billing & usage. + +**A section parent is a page, not an alias for its first child.** Users & access, Connections, AI Center & +MCP, Security, Billing & usage and Infrastructure each own an overview at their bare path: a card per child +carrying that child's live count and plan tier, then the few sections that only make sense across the whole +section (sign-in policy, connection health, AI credits, a security checklist, infrastructure status). +Children therefore all address an explicit `?tab=`, where the first one used to sit on the bare path. The +overview is what makes a parent worth clicking; without it the parent row was a lie, navigating somewhere +its label did not name. This also retires the rule below that a page with its own tabs never becomes a tab: +`Workers`, `Health`, `Triggers` and `Configurations` are now tabs of `Infrastructure`, which is what let the +group labels come back over a nav short enough to read. + +**Three knobs decide whether a row appears, and they are not interchangeable.** `isLocked` keeps the row +visible and crowned and the page reachable, because the point is to advertise what a plan would buy. +`isHidden` removes the row and the route together, for something an edition does not have at all rather +than has not bought: the AI `capabilities` tab is `isHidden` on Community, matching what +`setup/ai/index.tsx` did before the registry. `hideInNav` keeps the route and drops only the nav entry, for +a tab its parent row already stands for. Picking `isHidden` where `isLocked` belongs silently deletes a +feature from the product tour; picking `isLocked` where `isHidden` belongs advertises something the +edition can never enable. + +**`Pieces` takes the other way out: the parent row IS the first tab.** It has tabs and no overview, and +rather than invent a landing page over two children, its `pieces` tab carries `hideInNav: true`. The tab +stays routable and still owns the bare path, but the sidebar does not draw it as a sub-item, so the nav +reads `Pieces` with one child `Piece Sets`, and clicking the parent opens the pieces table. This satisfies +the rule above rather than breaking it: the parent row goes where its label says. Reach for it whenever a +section's first tab is the obvious thing the parent name already promises, and reach for an overview when +the parent name covers several children equally. + +**Those sections live in the sidebar, not in the page.** Each one is an indented sub-item under its parent, +with a connector line. The counts in this decision's title and first paragraphs were the plan; the registry +is the truth and has moved since, currently eleven nav parents and twenty children. An in-page tab row was built +first and rejected: it left the sidebar reading as fifteen flat items with nothing under them, while the +structure that mattered was hidden inside whichever page you had opened. Sub-items address the same +`?tab=` the tabs used, so the first section stays on the bare path and every redirect kept working. + +**A page that already has its own tabs does not become a tab** was the rule that decided most of the +shape, and it is now fully retired. It went first for Infrastructure, which absorbed `Workers`, `Health`, +`Triggers` and `Configurations`, and then for `Pieces`, whose own two tabs became the registry tabs +`Pieces` and `Piece Sets` while its `:id` child route stayed where it was. What survives of the rule is +its opposite: an in-page tab row is a **candidate** for promotion, because the sidebar is where sections +belong. `Templates` and `Embedding` stay separate for their own reasons, so the embed wizard keeps a page +of its own. `MCP Server` folds into `AI Center & MCP` as a third tab, because its own two tabs are a light +pill control that reads as a different level rather than a second tab row. + +**Promoting an in-page tab row costs no URL change**, which is what makes it cheap. Both conventions are +the same one: first tab on the bare path, the rest on `?tab=`. `Pieces` was already hand-rolling +exactly that, deleting a `useSearchParams` handler and a `TabsList` per page promoted. + +**Ask the registry which tab is active; never recompute it in the sidebar.** `adminPagesUtils.activeTabId` +resolves the `?tab=` param, or `null` when the page has an overview, or the first tab otherwise, and the +nav highlights the parent whenever no visible sub-item owns that answer. That one rule replaced three +hand-written special cases (`tabs.length === 0`, `hasOverview && tab === null`, `index === 0`), which had +all silently assumed every routable tab was also a visible sub-item. `hideInNav` broke that assumption, and +anything else that makes the routable set differ from the nav set will break it again. + +**Sample mode mounts the real page, so its plan-gated queries must be `enabled`-guarded.** `FeatureSample` renders the +locked page ghosted underneath the upgrade card, which means every `useQuery` inside it fires for platforms that lack +the feature; an endpoint behind `platformMustHaveFeatureEnabled` then answers 402 on every visit (seen 2026-09-14 on +`/platform/security/embed`: `GET /v1/signing-keys` → 402 until `useSigningKeys` got `enabled: platform.plan.embeddingEnabled`). +`LockedFeatureGuard` used to hide this by not mounting the page at all. A 31-page smoke run also surfaced three +pre-existing console warnings the ghosted rows now expose on every plan: a bare `` around a `` +(nested buttons, on projects / templates / project roles), `` wrapping a `<>` fragment in +`edit-global-connection-dialog.tsx` ("Invalid prop `type` supplied to React.Fragment"), and DataTable filters whose +`accessorKey` names no column (`pieceName`/`projectIds`/`ownerIds` on connections, `name` on pieces). None are regressions. +The populated states were verified separately against a 0.90.4 EE backend with seeded data (two pending platform +invitations, six connections with one in `ERROR`): "Pending invitations" lists the emails, "Needs attention" lists the +failing connection, "Most used pieces" ranks by count. One nit surfaced there: both connection overview sections label rows +with the raw package name (`@activepieces/piece-apollo`) instead of the piece display name. Seeding notes: the invitation +endpoint enforces the seat quota (402 `QUOTA_EXCEEDED`) so `platform_plan.usersLimit` has to be lifted first; a personal +project refuses project invitations ("Project must be a team project"); connections cannot be inserted by hand because the +list endpoint decrypts every `value` (`ERR_CRYPTO_INVALID_IV` → 500), and a dev backend with `AP_PIECES_SOURCE` on files +returns `piece_metadata_not_found` for anything not in `AP_DEV_PIECES`, so seed through `POST /v1/app-connections` with +`type: NO_AUTH` (skips engine validation) against a backend that knows the piece. + +**An overview is gated like any other page, an unknown tab redirects, and a failed query says so.** The overview +branch of the route resolver wraps in the same `FeatureSample` as component pages, so a locked section (Billing on +Community, declared with `sample: true` and a teaser in the registry) shows the upgrade card instead of empty values. +A `?tab=` that is hidden or unknown redirects to the page's bare path rather than silently rendering the first tab, +and overview cards derive their tab links from `visibleTabs`, never a hardcoded list. `OverviewCard` and +`OverviewSection` take `isError`/`error` and `isLoading`: a failed fetch renders `DataFetchErrorState`, a pending one a +skeleton, and only real data can claim "healthy" or "none". The connections overview counts project-scoped +connections only, so global ones are not counted twice, while "Needs attention" still lists failing global ones. +Greptile caught all four on the first review of #15526 (2026-09-14). + +**Groups never collapse.** Every page with sub-items draws them all the time; there is no chevron and no +collapse state. A per-page `collapsed` boolean was tried first and left `/platform/users?tab=members` with +nothing lit up whenever the group had been closed earlier; a per-navigation version fixed that but bought +little for its state, so it was dropped for always-expanded on 2026-09-13. + +**`General` keeps its name and keeps branding inside it.** It holds the platform name, the five plan-gated +branding assets, the auto-create-personal-projects default, and the Cloud owner's danger zone, as one form +with one Save. A `Branding` page was split out first and has been folded back: the split was there so each +of the two pages was wholly gated or not gated at all, but it cost a second nav item and a second form for +five inputs, and the page it left behind was three fields. + +The gated half is marked in place instead, with a `FeatureBanner` above the branding inputs carrying the +upgrade link while the platform name stays editable. That is the same treatment `Pieces` already uses for a +partly gated page. `General` therefore wears no crown and is the one admin page where part of the body is +gated and part is not. + +**A paid feature is gated at the action, not at the door.** The page opens, the data loads, the forms +fill in, and the crown sits on the primary action: on `New Project`, again on `Create Project` inside the +dialog, and only that final click is refused, with an upgrade dialog explaining the feature. An admin +evaluating Activepieces can therefore see what a feature is before being asked to pay for it, which the +earlier full-page teaser made impossible. + +**Which click refuses depends on whether the input can still change the answer.** `New Project` defers to +the final `Create Project` on purpose: the dialog is worth opening because seeing the form is part of +seeing the feature. `Invite` is the opposite case, because `isOutOfSeats` means zero seats remain, so +every invitation the admin could type is already going to fail; it refuses on the click and never opens +the form. Getting this wrong is a half-gate, marker on one control and check on another, which reads as a +gate and behaves as wasted typing. Ask whether any input reaches a different outcome. If not, refuse where +the crown is. + +**The crown replaces the action's own icon, it does not join it.** A plus and a crown side by side read as +two separate affordances. `useFeatureGate` returns `crown` as an element when locked and `null` when not, +so a plain button is `{gate.crown ?? }` and only ever shows one icon. The +exception is `AnimatedIconButton`, whose icon is a prop rather than a child because it animates on hover: +a crown has no animation to drive, so the locked case renders a plain `Button` instead. + +`gate.crown` carries `text-primary-foreground/90`, which is white, so it only reads on the purple primary +button it was written for and disappears on an `outline` one. `Customize Selector` is outline and renders +its own `` instead, matching the sidebar crowns, while +still taking `open` and `dialog` from the hook. Take the behaviour from the gate and the colour from the +button until the helper learns to inherit `currentColor`. + +**A refused submit swaps its dialog's content, it does not trade one dialog for another.** Closing the form +and opening a second dialog was tried and rejected: each Radix `Dialog` owns an overlay, so the first +animates out while the second animates in, the backdrop undims between them and the screen visibly +flashes. Stacking is worse still, leaving a dead form dimmed behind an upsell of a different width. So the +gate hands over *content* and the host dialog renders it in place of its form, and the overlay element +never unmounts. Two things to get right: reset the swap flag through the same handler that sets `open`, +since a footer button calling `setOpen(false)` skips `onOpenChange` and the next open would show the upsell +instead of the form; and check any two-dialog interaction in the browser, because that is where Radix +orphans `pointer-events: none` on `` and the page behind stops taking clicks. + +**A feature you cannot act on renders as the real page, and the gate moves to the write.** +`Audit logs` is a table, so there is no action to crown, and opening it freely would show an empty table +with nothing to explain why. So the page renders at full opacity with nothing over it: real header, real +toolbar, real rows filled with the sample fixtures. Reads all work against those fixtures, meaning search, +filters, sorting, pagination and opening a row for its payload. A write, meaning Save, New, Delete, Export +or a config switch, opens an upgrade popover anchored to the control that was pressed. There is no overlay +and no scrim anywhere in the flow. + +Three non-blocking marks say whose data this is: a `Sample data` pill beside the page title carrying the +tier, the table header tinted `#f6f3fe` over the data region only, and a rail pinned to the bottom of the +page holding the one-line pitch and the link out to what the plan adds. On a form page that rail merges +with the form's own action row, so Discard and Save sit inside it. + +Designed in Paper, not yet built: the branch still ships the centred sign-in card described below. + +**The presentation was iterated seven times, and the rejected six are the useful record**, because each +failed for a reason worth not rediscovering. The seventh, the centred **sign-in card** over a quarter +opacity page, is the one this supersedes: it keeps the whole page visible where a fade shows only its top +third, but it still puts a wall between the person and the thing they came to look at. **Blur** hides the +very rows the treatment exists to show. A **strip** above the content said the right words but left the +page reading as ordinary. A **framed container** read as locked but dominated the page it was previewing. **Chrome merged into the page's own +header** was too quiet, and needed a React context plus edits to both shared headers and `DataTable` to +deliver. A **full-bleed banner** announced the paywall before the page had shown anything. A **gradient +fade** read as one surface but revealed only the top of a page and cut a form mid-field. A Mobbin survey +found products split between crisp-and-labelled and fade-an-empty-shell, and none sampling a form, so +there is no direct precedent to defer to here. + +**The sample content is inert, and that is load bearing.** An "explore freely, upgrade on write" version +was designed, where filters filter and sorting sorts against the fixtures and only a write is refused. It +was not built. What ships ghosts the whole region at a quarter opacity and refuses every interaction, +because the fixtures are rendered by the real components: the controls in there are the live ones, wired +to the live mutations. + +**Ghosting is not withholding.** `LockedFeatureGuard` returned a teaser *instead of* its children, so +nothing was mounted. `FeatureSample` mounts them and dims them, which is what makes the preview worth +looking at and also what puts live controls in the page. `aria-hidden` and `pointer-events: none` do not +remove anything from the tab order, so Tab reached those controls and Enter activated them, and a +portalled dialog then rendered *outside* the ghosted subtree at full opacity. `inert` is the attribute that +actually removes a subtree from focus, pointer and accessibility handling; `pointer-events: none` stays +underneath it only as the fallback for browsers without it. Treat the server as a backstop you have not +verified rather than one you have, and never reintroduce interactivity here without re-deciding this. + +Sample data is **generated in the browser and never crosses the API**. A locked page skips its plan-gated +query outright rather than letting it answer 402, and the endpoints keep refusing. Serving fabricated rows +from a gated endpoint was rejected outright: an audit trail that returns fiction is a support and +compliance hazard, and every API key and script would see it too. + +So the rule is: crown the action where there is one, and fill the surface with sample data where there is +not. + +The nav still marks a page when **every** section on it is gated, and marks a gated **section** directly, +so `Security` stays uncrowned while its API keys section works and its three paid sections each carry a +crown. + +**The route resolves `?tab=`; every section keeps its own header.** The registry entry names the sections, +the route element picks the active one and renders that component directly, so no page hand-rolls the +searchParams dance again. But the header stays with the page: a unified `AdminPage` shell was built first +and rejected, because it stretched the heading across the full page width instead of the content column, +and it stacked the parent above the section as "AI Center & MCP · Capabilities", which is a breadcrumb the +sidebar already draws. Each page renders the `DashboardPageHeader` or `CenteredPage` header it had before, +and the sections that never had one got the same treatment as their neighbours. The parent name lives only +in the sidebar. `AI Center & MCP` lost the vertical rail it used to ship with, since the sidebar now does +that job for every page. Community sees the same structure as everyone else, but a gated tab there gets a +short "available in Enterprise" note with a docs link rather than benefit bullets and a sales button. + +It lands as four PRs: the registry and shell with **no visible change** at all, then the cheap merges, +then gating, then the collapse with its redirects and docs. `/platform` keeps redirecting to Projects: the +section overviews cover the "where am I" job, so a whole-platform landing page is still deferred. + +## Context + +22 nav items, 18,588 LOC across 103 files, and 13 items wearing a lock icon that is decorative: the click +navigates regardless and the destination page is the real gate. What that page does is inconsistent by +accident, not design. Twelve pages replace their whole body with `LockedFeatureGuard`, Pieces stays usable +under a banner, the branding fields just set `disabled` with no explanation and no upgrade path at all, +and AI Center gates on platform role rather than a plan flag. + +The nav is a literal inside `PlatformSidebar`, the routes are a flat hand-written array, and each page's +gate is hardcoded in its own file. Nothing ties the three together, which is why `Embedding` sits under +Setup while routing to `/platform/security/`, and `Event Streaming` sits under Observability while routing +to `/platform/infrastructure/`. + +The consolidation itself is not new here. AI Center already folded two nav items into one tabbed page with +a validated `?tab=`, MCP Server was built tab-first, and `setup/general` composes two sections on one page. +Four pages now hand-roll the same searchParams dance, three with an unsafe cast. + +## Why + +- **Fifteen, not the eleven the mockup drew.** The line counts and the existing tab rows decide it. `usage` is 47 lines and already sits beside billing on + the same gate and the same endpoint, `mcp` is 117, and sso plus api-keys plus project-roles are 401 and + already the same `Item` list shape. Against that, Pieces and AI Center are ~3,000 LOC each with their own + tabs, so folding them in buys a smaller nav by making two real pages harder to use. +- **One registry over three sources.** Adding an admin page becomes one entry instead of three edits, the + group and URL can no longer disagree, and the nav crown becomes a derived value rather than a maintained + boolean. The cost accepted is one file that every admin change touches. +- **A shell that owns tabs, not the headers.** Eight pages are about to need the tab dance that four pages + already copy and three get subtly wrong, so the routing layer absorbs it. Absorbing the headers too was + the overreach: the twenty-odd pages already agree on two header shapes, and centralising them traded a + layout every page had right for one the shell got wrong everywhere at once. +- **Three treatments, chosen by what the surface offers.** Once pages carry tabs, a full-page takeover is + wrong by construction: it blanks a page whose other tabs work. So a page with a primary action gets a + crown on it, a page you only read gets its own UI blurred, and a page that is partly gated keeps working + under a `FeatureBanner`. One rule for all three would either hide working controls behind an upsell or + leave an empty greyed table reading as broken. +- **Crown only when the whole page is gated.** A crown on `Users & access` when only SCIM is paid would be a + lie, because Members works. Bundling gated tabs beside free ones is what drops thirteen markers to three. + `Projects` and `Pieces` shipped breaking this and were corrected on 2026-09-14: both pages load and read + fine unpaid, and only one control on each is gated, so the page-level `isLocked` came off and the crown + moved onto `New Project` and `Customize Selector`. The test is whether a person who cannot pay still has + a reason to open the page. If yes, the sidebar stays clean and the control carries the mark. +- **Four PRs, each reviewing one idea.** PR #14330 reverted the last big navigation rework, but that revert + touched **zero** platform-admin files: 106 files across global search, the builder canvas and the dashboard + sidebar. Its real lesson is that it bundled a navigation model change with unrelated surfaces, so nothing + could be reverted alone. Scoping this to platform admin and splitting by concern keeps each piece + revertable. Rendering the platform nav's item chrome locally rather than editing the shared + `ap-sidebar-item` keeps the shared-file count at zero, at the cost of having to re-implement everything + that component quietly did. The sidebar icons are the bill: each one in `components/icons/` is a + `forwardRef` exposing `startAnimation` / `stopAnimation`, and `ApSidebarItem` drives them from row hover + through a ref. `PlatformNavItem` rendered the same components with no ref, so the icons went inert and + nothing failed. `pointer-events-none` on the icon is why the self-animating fallback does not cover it + either: the icon's own `onMouseEnter` never fires, and the component only marks itself externally + controlled once a ref is attached. Restored 2026-09-15. Before copying a shared component's markup, + diff it for refs, effects and handlers, not just classes. +- **A quieter Community.** This is the open-source product, and upsell density there is a brand question and + not only a conversion one. The structure stays honest, the pitch does not follow self-hosters around. +- **No landing page yet.** It is purely additive, blocks nothing, needs five or six separate queries with no + aggregate endpoint, and is far easier to design once the twelve-page IA exists. + +## Consequences + +**A tier label has to name a plan somebody can buy.** The gating UI carries its own two-value vocabulary, +`FeatureTier = 'team' | 'enterprise'`, entirely separate from `PlanName` in shared. It shipped as +`'team' | 'ultimate'`, and there is no Ultimate plan: billing sells Free, Plus, Team and Enterprise. So a +locked page told a Plus admin to "Upgrade to Ultimate", and the Explore plans dialog it opened offered no +such thing. Renamed to `enterprise` on 2026-09-14. The tier is decorative, meaning nothing maps it to a +plan id or preselects a plan at checkout, which is exactly why the drift went unnoticed; keep the two +vocabularies spelled the same so the next drift is visible. `FeatureTier` and `TIER_LABELS` are still +declared in four places (`lib/feature-tier.ts`, `feature-teaser.tsx`, `use-feature-gate.tsx`, and a fourth +label map inline in `overview-shell.tsx`), so any future rename has to touch all four. + +**The registry trades drift bugs for derivation bugs, and the escape hatches are how you tell whether it +is still worth it.** Three sources describing the same pages had already disagreed in production, which is +why this exists, and that class is now impossible: the URL and the nav entry are one object. What replaced +it is smaller but real, and the first week produced two of them. The registry claimed `?tab=` and silently +broke the two pages that kept their own inner tabs there, and `isCrowned` miscomputed a page whose lock +lived on the page rather than its tabs. A hand-maintained boolean can be stale; it cannot be computed +wrong. Expect that shape of bug from anything derived here. + +The signal to watch is the **escape-hatch count**. `AdminPage` carries `isHidden`, `hideInNav`, `overview` +versus `component` versus `tabs`: four places where the uniform model did not hold, one of which +(`hideInNav`) was added days after the registry landed because Pieces wanted a routable tab that the nav +should not draw. Each new one is the model being bent rather than used. If the list keeps growing, the +answer is not a fifth: it is a thinner registry carrying path, title, nav and gate only, with routes +composed normally, which keeps most of the benefit because the drift was always between *those* four +things and never between the JSX blocks. + +**An overview's card row fits three, and the fourth wraps.** The cards sit in a +`repeat(auto-fit, minmax(15rem, 1fr))` grid inside a column capped at 1024px, so three tracks plus gaps +are all that fit and Security and Infrastructure render three cards plus an orphan. Narrowing the track +until four fit truncates the longer titles (`Secret mana...`, `Event strea...`), which is worse than the +wrap, so the wrap stands. Treat four as the point where a section has outgrown a single card row rather +than something to squeeze. Use `auto-fit`, never `auto-fill`: `auto-fill` keeps the empty tracks alive, so +a two-card overview renders both at a fifth of the width with dead space beside them. + +**A gated query with no `enabled:` puts an error state behind the blur.** The overlay works only because +the query underneath already carries `enabled: platform.plan.`, the repo rule for any endpoint behind +`platformMustHaveFeatureEnabled`. Without it the request fires, answers 402, and `DataFetchErrorState` +renders "Trouble loading …" under the glass, which reads as broken rather than unpurchased. Check that +guard before giving any surface an overlay. + +**PR 0 describes today's 22-item IA on purpose.** The registry and shell ship deriving the nav and routes +that already exist, so the diff is reviewable as pure refactor with identical screenshots, and the +derivation is proven against the IA everyone knows before the data changes under it. + +**The quiet Community treatment was reversed in review: `RequestTrial` stays.** Retiring it would have +closed the only in-product path from a self-hosted install to sales, feeding `activepieces.com/sales` from +26 feature keys, and doing that inside a design refactor made it a growth decision smuggled in as one. Both +locked surfaces render it again on Community, gated by `showContactSales`, which only the billing shell +sets to `false` exactly as before. Each registry teaser therefore carries the `featureKey` the sales page +expects; a surface with no teaser shows no form rather than one tagged with the wrong feature. + +Old deep links must redirect. 22 items becoming 15 means eleven redirects on top of the five +that exist, and `docs/docs.json` mirrors this IA across about twenty `admin-guide` pages, so a rename lands +in the same PR as the route change. + +`SidebarProvider open={true}` means the admin sidebar never collapses, so the collapsed-state branches in +the shared item component stay unreachable from here. + +PR #15028 tuned the admin nav's widths and spacing to line up with the primary rail by hand, and the rail +imports none of the sidebar primitives. Any change to admin nav metrics needs re-checking against the rail, +and nothing will fail if it drifts. + +**Folding Branding back into `General` reopens the per-field `brandingLocked` gotcha**, where five inputs +grey out beside a sixth that works. The banner above them is what keeps it from being the silent version +the brain records: it names the feature and carries the upgrade link, so the disabled inputs are explained +rather than merely dead. Whether that is enough on Community, which now sees an uncrowned `General` with a +banner inside it instead of a crowned `Branding` item, has not been reviewed yet. Revisit before the gating +PR ships. + +Two dead things went with the split: the three file inputs were bound to `logoUrl` / `iconUrl` / +`faviconUrl` form fields whose values nothing ever read, since the inputs are ref-driven and render +`platform.fullLogoUrl` directly, and `sampleData.branding` was their only consumer so it never rendered +either. Both are gone. + +**A tab promoted to a section inherits nothing from its old page wrapper.** The AI Center page supplied +`max-w-6xl px-8 py-6` and the scroll container for its tabs, so `providers-tab` and `capabilities-tab` +carried no padding of their own; once the route rendered them directly they sat flush against the edge. +Each section now supplies its own padding, and the layout's `#dashboard-content-container` already +scrolls, so none of them needs `overflow-auto`. Check the old wrapper before promoting the next tab. + +The same bite has a second shape: whatever the old wrapper rendered **above** the tab row now renders on +no tab at all. The Pieces page held one `DashboardPageHeader` and one `FeatureBanner` for both tabs, so +promoting them meant giving each tab its own header and extracting the banner into `PiecesLockedBanner` +for both to call. A banner is the easy one to miss, because losing it leaves the locked tab silently empty +rather than visibly broken. + +**The third shape is the dangerous one: `?tab=` is a single namespace, and a promoted page that keeps its +own inner tab row in the URL collides with the registry.** `infra/health` reads +`searchParams.get('tab')` for its `system | runs | queue` row and `infra/workers` for its +`health | worker-groups` row. Once Infrastructure became a tabbed registry page the router started writing +`?tab=health`, the page read that same param, matched none of its own panels, and rendered a tab row with +nothing under it. Worse, clicking an inner tab writes `?tab=runs`, which is not an Infrastructure tab id, +so the route's unknown-tab guard redirects to the bare path and ejects you to the overview. Triggers and +Configurations survived only because they have no inner tabs. + +Grep the page for `searchParams.get('tab')` before promoting it. **The registry owns `?tab=`; a promoted +page's own row moves to `?view=`**, exported as `VIEW_QUERY_PARAM` from `admin-pages.ts` so the contract +sits beside the thing that claims the other half. Promoting the inner tabs instead was the alternative and +was rejected: it puts six children under one parent, which is what the collapse existed to avoid. Fixed for +Health and Workers on 2026-09-14; old inner-tab deep links like `?tab=runs` now land on the parent overview +rather than a blank page. + +Both pages had reached for the param with `searchParams.get('tab') as TabValue`, and that cast is what let +`'health'` through as a legal value of a `system | runs | queue` union. A `parseTabValue` guard that falls +back to the default is what makes an unknown param render the first panel instead of nothing, which is the +difference between a wrong link looking wrong and looking broken. The repo bans `as` casts for exactly this. + +**A parent crown is the AND of its tabs**, since `isCrowned` returns true only when every visible tab is +locked. A page whose tabs share one plan flag therefore has to repeat that flag on each tab to keep the +crown it had as a single item: `Pieces` carries `managePiecesEnabled` on both of its. + +**Do not give a section a page header when it already opens with a `SectionHeader`.** Providers and +Capabilities each led with one, so adding a header above it printed the title twice. + +**Sampling every locked page costs two working controls on Single sign on.** `ssoEnabled` gates only +SAML: `platform.service.ts` checks the plan solely when the payload carries `federatedAuthProviders.saml`, +and `googleAuthEnabled` is enforced with no plan check at all, so the Google and Allowed Email Login +toggles genuinely work on a plan without SSO. Sampling the section wholesale, rather than only its SAML +row, therefore takes two live controls away from EE and Cloud admins who have them today. Taken knowingly, +for one consistent treatment across all nine locked pages. Community is unaffected, because its +sign-in enforcement returns early on that edition anyway. + +**Sample data does not mean inventing fixtures for audit events.** `buildMockEvent` in +`@activepieces/shared` already builds realistic `ApplicationEvent` values across forty event names, and the +web app already calls it for the test-destination flow. Reuse it there; only the other locked surfaces need +fixtures written by hand. + +**Community gets its own call to action inside the loud treatment.** It sees the same sample content, but +where paid editions show `Upgrade to {tier}` and an `Included with the {tier} plan and above.` footer, +Community gets Contact Sales above a docs link, because a self-hosted install has no plan to buy in-app. +The tier is named only on the paid branch, so a Community reader is told what the feature is but never +which plan it belongs to. + +**Nothing renders the full-page teaser once all nine are sampled.** `LockedFeatureGuard` and the page-level +`FeatureTeaser` lose their last callers in platform admin, and `overlay: true` in the registry is replaced +by the sample opt-in. `FeatureTeaserContent` survives inside the upgrade dialog. + +**One form, not two.** `appearance-section.tsx` is a single react-hook-form whose submit handler +deliberately skips the branding fields when locked, and both halves post to the same +`platformApi.updateWithFormData`. Splitting it was tried and undone; keeping it whole is what gives the page +one Save. + +`Configurations`, added to the nav on 2026-09-07 after the proposal was drawn, becomes Infrastructure's +fourth tab and carries its Cloud exclusion into its registry entry. It already routed under +`/platform/infrastructure/`, so nothing moves. The credits meter the admin sidebar lacks is deferred with +the overview page: additive, and `SidebarUsageLimits` already exists and already returns nothing on +Community. + +The four PRs are scoped in `packages/web`, but the largest single chunk is not there. PR 3 carries roughly +fourteen route redirects plus the `admin-guide` restructure in `docs/docs.json`, about twenty pages that +PR #15153 had just expanded. + +## Tracking + +Grilled 2026-09-07 against the `maybe-chat.activepieces.ai/docs/admin-nav` concept and Langdock's settings +IA as references. + +The sample-data treatment was checked against Mobbin afterwards, and the survey supports dropping the +blur: every product found picks one side or the other, never both. Later and Churnkey show crisp, +realistic figures under a labelled band ("just a glimpse", "ANALYTICS PREVIEW"), while Juicebox, Asana +Admin and Hex fade an essentially empty shell behind a card and teach the reader nothing about the +feature. Churnkey's one-word "preview" also reads better than a sentence explaining that the data is not +real. No product in the survey samples a *form*, so our Single sign on, General and Embedding treatment +has no precedent to lean on, which is worth remembering given it is the same part that costs two working +controls. diff --git a/brain/knowledge/engineering/ci-pr-review-hygiene.md b/brain/knowledge/engineering/ci-pr-review-hygiene.md index 00a16b5ebcef..bb5df7218db4 100644 --- a/brain/knowledge/engineering/ci-pr-review-hygiene.md +++ b/brain/knowledge/engineering/ci-pr-review-hygiene.md @@ -10,7 +10,9 @@ The CI gates that shape *how a PR is reviewed*, as opposed to whether it builds. We open PRs as drafts so no human reviewer is auto-assigned until "Ready for review". Greptile's **Review draft pull requests** setting is enabled, so its first pass lands on draft open with no CI glue — first-pass AI review while it is still a draft, human review after. Unlike a once-per-PR CI nudge, the native setting also re-reviews as commits land on the draft. ## Per-area size gate -`pr-size.yml` + `tools/scripts/pr-size-check.ts` count **meaningful lines** (additions + deletions, minus lockfiles, `i18n/translation.json`, `locales/**`, snapshots, `dist`) per area and fail when a gated area is over budget: engine+worker+execution combined 300, `core/shared` 250, `server/api` 600, `packages/web` 1200. `packages/pieces` and everything unmatched are measured but exempt — a line count can't tell a cohesive new piece from a codemod, and pieces are self-contained with low blast radius. Bypass with the `large-pr-ok` label or a `revert:` title. Budgets were calibrated from the distribution of recently merged PRs. +`pr-size.yml` + `tools/scripts/pr-size-check.ts` count **meaningful lines** (additions + deletions, minus lockfiles, `i18n/translation.json`, `locales/**`, snapshots, `dist`) per area and fail when a gated area is over budget: engine+worker+execution combined 300, `core/shared` 250, `server/api` 600, `packages/web` 1200. `packages/pieces`, `tests` and everything unmatched are measured but exempt — a line count can't tell a cohesive new piece from a codemod, and pieces are self-contained with low blast radius. Bypass with the `large-pr-ok` label or a `revert:` title. Budgets were calibrated from the distribution of recently merged PRs. + +**Tests are their own area and are never charged to the code they cover.** `TEST_PATTERNS` is the first bucket, and first match wins, so `packages/server/api/test/**` lands in `tests`, not `server/api`. It matches a `test`/`tests`/`__tests__`/`__mocks__`/`__snapshots__` path *segment*, a `.test.`/`.spec.` suffix, a `vitest`/`jest`/`playwright`/`checkly` config, `packages/tests-e2e/`, and `smoke-test/`. Otherwise the gate taxes the one thing it should be encouraging: 60% of recently merged PRs carry test lines, and 8 of the last 150 were over budget *only* because of theirs. The budgets were calibrated against diffs that included tests, so this makes them somewhat looser than measured — retune from the new distribution rather than assuming the old numbers still bind the same way. The diff comes from local `git diff --numstat`, not the `/files` API, so it is immune to GitHub's 3,000-file response cap — a mega-PR cannot under-count its way past the gate. @@ -20,6 +22,7 @@ Which team gets asked to review comes entirely from `.github/CODEOWNERS` — the Enforcement is the **`Codeowners review` repository ruleset** (active on the default branch), not classic branch protection: `require_code_owner_review: true` plus `required_approving_review_count: 1` and `required_review_thread_resolution: true`. Eight bypass actors are configured, which is why an owner-team request can look non-blocking on some PRs. ## Gotchas +- **The tests bucket keys off a path *segment*, so a feature folder named `test-step` is not mistaken for tests.** `packages/web/src/app/builder/test-step/` and `latest-test.ts` stay in `packages/web`; the regex requires `test/` as a whole segment or a `.test.`/`.spec.` suffix. The one deliberate overlap is `packages/pieces/framework/src/lib/test/index.ts` — shipped `createMockActionContext` helpers, counted as tests, and exempt either way because pieces are exempt. - **A job-level `env:` in `ci.yml` does not reach anything turbo runs.** Turbo 2.x defaults to `--env-mode=strict`: a task's process only receives variables named in `globalPassThroughEnv` / `passThroughEnv` / `env`, plus turbo's own system list. Everything else is dropped silently, so the task falls back to whatever `packages/server/api/.env.tests` set (dotenv there runs without `override`, so a real env var would otherwise win). Symptom: the workflow clearly sets the variable and the test still uses the old value. Add the name to `globalPassThroughEnv` in `turbo.json` — that is why `AP_DB_TYPE` and `AP_POSTGRES_*` are already listed. Jobs that call `npx vitest` directly, like `tool-search-postgres`, are unaffected, which is what makes the difference easy to miss. Verify with a throwaway test that `console.log`s the variable through `npx turbo run --filter=`. - **A GitHub service container is not reachable by its name from a non-container job.** `services:` attaches the container to a docker network with a `--network-alias`, but the runner itself is on the host, so the alias does not resolve (`getaddrinfo EAI_AGAIN redis`) and only the published `ports:` mapping on `localhost` works. `.env.tests` pins `AP_REDIS_HOST=redis` for `docker-compose`, so any test that opens a real connection needs `localhost` forced past it. - **Never use `git stash` to prove a new test fails without its fix. Use `git checkout -- ` instead.** `git stash push -- ` on a path with no uncommitted changes saves nothing and creates no entry, so a following `git stash pop` silently pops whoever's stash is at `stash@{0}` instead. This repo carries long-lived stashes from other branches, so the pop conflicts, is kept, and still writes that stash's untracked files into the working tree, which then look like your own new files. It has happened at least twice, and `stash@{1}` is literally named *"recovered: AGENTS.md agent-skills section (accidentally popped by claude)"*. Reverting one committed file to its base version and running the test there is the same proof with no shared state: `git checkout -- `, run, then `git checkout HEAD -- `. If a stash pop does go wrong, the entry survives the conflict, so the recovery is to delete the stray untracked files after confirming they belong to it with `git stash show --include-untracked --name-only stash@{0}`. @@ -48,6 +51,7 @@ Enforcement is the **`Codeowners review` repository ruleset** (active on the def - **`@activepieces/shared` re-exports from `@activepieces/core-execution`, so a partial rebuild produces phantom "has no exported member" errors in unrelated files.** Rebuilding `core/shared` against a stale `core/execution` dist drops those re-exports, and the API typecheck then fails in `ee/agent/*` on symbols like `GetPersonalizationConfigRequest` — which live in `core/execution/src/lib/workers/worker-contract.ts`, not in shared at all. It reads exactly like a bad merge. The dependency order that actually works is `core/utils` → `core/piece-types` → `core/formula` → `core/execution` → `core/shared` → `server/utils` → `pieces/framework` → `core/ai-providers`; skipping a link silently poisons everything downstream of it. The same staleness makes an editor report missing enum members that exist in the source. **There is no api typecheck "error baseline" — rebuild first, then trust it.** A stale `core/shared/dist` reliably yields a handful of `ee/agent` errors that look permanent enough to be waved off as known, which is how a real error hides among them; after `npx turbo run build --filter=@activepieces/shared`, `tsc --noEmit -p packages/server/api/tsconfig.app.json` comes back completely clean. The same staleness invents errors in files a rebase just touched, so during a conflict resolution rebuild before concluding you resolved it wrong. - **To pull a file back out of a PR, restore it from the merge-base, never from `origin/main`.** A PR's diff is computed against the merge-base, so `git checkout origin/main -- ` does not "revert" the file — it imports every change `main` made to it since the fork and attributes them to you. Dropping one web file from [#15001](https://github.com/activepieces/activepieces/pull/15001) that way would have silently added 82 insertions / 44 deletions of somebody else's work. `git checkout $(git merge-base origin/main HEAD) -- ` makes it byte-identical to where the branch started, so it leaves the diff entirely and merges cleanly instead of conflicting. Verify with `git diff --quiet $(git merge-base origin/main HEAD) -- ` before committing, and read `git status` first — a `bun.lock` left dirty by an earlier `bun install` loves to ride along on a commit like this. - **Retargeting a stacked PR to `main` does not drop its base branch — it merges the whole thing.** A PR opened against a long-lived feature branch shows a small diff *relative to that base*, but `gh pr edit --base main` only moves the target; the branch still contains every commit of its old base. [#14593](https://github.com/activepieces/activepieces/pull/14593) read as 2 docs files against `feat/autumn-billing-integration` and as 198 commits / 211 files / +12k lines against `main`. Check with `git diff --stat origin/main...` **before** retargeting, and if it disagrees with the PR page, cherry-pick that PR's own commits onto `main` and force-push instead. A "conflict" on such a PR is often against the feature base only — those same commits can apply to `main` cleanly. +- **Each link of a stack runs CI against its own base, so verifying the top proves nothing about the ones below it.** A refactor that lands on the bottom branch and is repaired two branches up is green where you tested and red on the PR that actually carries it, and the failure reads as unrelated because the file it names was fine when you last looked at it. Before pushing a stack, check out each branch in turn and run the package's typecheck, lint and full test suite there, not just at the tip; the suites legitimately differ (this stack's tip runs two more web tests than its base). Do the same after any rebase, since a conflict resolved in one replay can leave a lower branch importing something only a higher one defines. - **A deliberately stacked PR is charged only for its own delta, but each link pays its own `@activepieces/shared` bump.** `pr-size-check.ts` diffs `HEAD^1...HEAD^2` on the `pull_request` merge commit precisely so an ancestor PR is not re-counted — a stack member measures the same whether reviewed against `main` or against the PR below it, so splitting a large change into a stack really does buy budget rather than just moving it. Two things the split still costs you. Every PR in the stack that touches `packages/core/shared` needs its **own** version increment (0.158.0 then 0.159.0, not the same bump twice), and `bun.lock` records each workspace's version, so edit that one line **by hand** in each — regenerating sweeps `main`'s pending drift into your diff, per the regenerator trap above. And prove the split is faithful before pushing: tag the pre-split commit, then `git diff ..` must come back empty apart from those intentional version lines. A file that lands partially in one PR and grows in the next (a service that gains its read side, a test file that gains its endpoint cases) is exactly where a hand-split silently drops a hunk. - **A migration-check failure in a stack names the PR that *introduced* the file, which is usually not the branch you are sitting on.** `check-migration-rollback.ts` lists candidates with `git diff --name-only --diff-filter=A origin/$GITHUB_BASE_REF...HEAD`, so a migration is only ever scanned on the stack member that **adds** it; every branch above inherits the file and its CI stays green on that check. So the pasted failure belongs to the lowest link, and editing the file on the tip you happen to have checked out fixes nothing the gate looks at. Confirm ownership before touching anything — `gh pr view --json headRefName` for the branch the number really points at, then `git cat-file -e :` across the stack for the first branch holding the file — fix it there, and rebase upward. Two things make that cheap: the file is usually byte-identical across the stack (`git diff -- ` comes back empty), and `git rebase` reports `skipped previously applied commit` for the lower branch's pre-rebase commits, which is the expected signal rather than a lost commit. Verify each link the same way the split rule above does — tag the old tip, then `git diff ` must show only the intended delta. Hit on PR 15240 (`feat/mcp-activity-recording`) while its own migration read as a failure on `feat/mcp-activity-serve` two links up. - **A decision authored on a long-lived branch will collide on its number.** `brain/decisions/` numbers are assigned once and never reused, but the next free number is only knowable against `main` — two branches in flight both grab it. #14593 carried a `000024` that `main` had since filled, and `000025` too, so it landed as `000026`. Renumber against `main` at merge time and update every referring link; nothing in CI catches a duplicate number or a dead decision link. @@ -75,3 +79,5 @@ Enforcement is the **`Codeowners review` repository ruleset** (active on the def - **Merging up a stacked chain, a parent's rename reaches a child's call sites as a clean auto-merge — only the typecheck finds it.** Git conflicts on lines both sides edited, so a symbol renamed on the parent merges silently into a child that merely *calls* the old name: `feat/barrier-service` renamed `barrierService.receive` → `receiveSignal`, and `feat/barrier-edges`' `resume-controller.ts` kept calling `.receive(...)` with no conflict marker. Worse, both sides can each be right and still merge wrong — the same commit renamed `findPreCompletedByFlowRunId` while the child had rewritten that function's *body* to fix a real bug, so the conflict exposed only the signature and taking either side whole silently dropped work. Two habits: typecheck **every** branch in the chain after its merge rather than trusting a conflict-free result, and before picking a side run `git log --oneline $(git merge-base HEAD MERGE_HEAD)..HEAD -- ` on both to read what each side was actually doing. Stacks here run deep (the waitpoints → process-in-batches chain was 17 PRs), so one missed rename propagates through every branch above it. - **Docs videos belong on `cdn.activepieces.com/videos/docs/`, never in the repo — and `docs/resources/` is the legacy path that suggests otherwise.** Every `