# Review Architecture

> Use when reviewing or designing shaka-perf framework extension points, pipeline or stage variants, artifact ownership, report serialization boundaries, or name-keyed behavior dispatch.

- Skill: `shakacode/review-architecture` (Agent Skill)
- Install (CLI): `npx skillmds@latest add shakacode/review-architecture`
- Raw SKILL.md: https://api.skillmd.com/api/skills/shakacode/review-architecture/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: DevOps & Infra
- Author: shakacode (https://skillmd.com/u/shakacode)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/shakacode/review-architecture

---


# Review Architecture

shaka-perf's framework primitives (`Pipeline`, `Stage`, …) are designed to be
extended polymorphically. Variant-specific behaviour belongs **inside the
variant**, surfaced through the framework via mandatory factory options.
The framework then calls polymorphic methods — never inspects `name` to
decide what to do.

This skill is both a design rule for new code and a review checklist for
existing code.

## The primitives this applies to

The same rule governs every variant-bearing primitive in shaka-perf:

- **`Pipeline`** — variants `audit`, `compare`. Created via
  `createPipeline({ name, report, pipelineConfig, … }, define)`. Variant
  behaviour lives on the returned `Pipeline` (chip builder, `report`
  renderers, …).
- **`Stage`** — variants `visreg`, `perf-warmup`, `perf`, `perf-low-noise`,
  `audit`. Created either by factory functions (`createVisregStage`) or by
  classes implementing the `Stage` interface (`PerfEngineStage`,
  `AuditStage`). The runner calls `stage.applies()`, `stage.run()`,
  `stage.renderArtifacts()`, `stage.machineReadableSummary()` polymorphically
  and never branches on `stage.name`.
- **Future primitives** added under `pipeline/` or `stage/` follow the same
  contract: variant behaviour on the variant, no central name switch.

```mermaid
classDiagram
  direction LR

  class createCompareCommand {
    <<function>>
    loads config
    creates pipeline
    calls runPipeline()
  }

  class createComparePipeline {
    <<function>>
    calls createPipeline()
    registers stages
    registers chipsForTest()
  }

  class Pipeline {
    <<interface>>
    name
    steps
    stages
    chipsForTest()
  }

  class Stage {
    <<interface>>
    name
    category
    applies()
    run()
    renderArtifacts()
    machineReadableSummary()
  }

  class createVisregStage {
    <<function>>
    returns Stage
  }

  class PerfEngineStage {
    <<class>>
    implements Stage
  }

  class runPipeline {
    <<function>>
    selects stages
    executes stage.run()
    persists outcomes
    assembles report data
  }

  class ArtifactStore {
    <<class>>
    writeOutcome()
    readOutcomesForViewport()
  }

  class writeReport {
    <<function>>
    writes report.html
  }

  class writeMachineReport {
    <<function>>
    writes report.json
  }

  class App {
    <<React component>>
    reads ReportData
    renders filters and test cards
  }

  createCompareCommand --> createComparePipeline
  createCompareCommand --> runPipeline

  createComparePipeline --> Pipeline
  createComparePipeline --> createVisregStage
  createComparePipeline --> PerfEngineStage
  Pipeline *-- Stage

  createVisregStage ..|> Stage
  PerfEngineStage ..|> Stage

  runPipeline --> Pipeline
  runPipeline --> Stage
  runPipeline --> ArtifactStore
  runPipeline --> writeReport
  runPipeline --> writeMachineReport

  writeReport --> App
```

The diagram is the rule in pictures: `runPipeline` only talks to `Pipeline`
and `Stage`. It never knows whether the stage is `createVisregStage` or
`PerfEngineStage`, and never knows whether the pipeline is audit or compare.
Adding a new variant is two new factory-side nodes plus implementations of
the same interfaces — zero edits to `runPipeline`, `App`, `writeReport`,
or `writeMachineReport`.

## When to apply

- Adding a new pipeline (`createXxxPipeline`) or stage variant.
- Adding a new extension point that several variants must implement
  (renderers, summary builders, chip producers, etc.).
- Reviewing a PR that introduces or modifies variant-specific behaviour.
- Auditing any `switch (pipelineName)` / `switch (stageName)` / similar
  name-keyed dispatch in shared modules.

## The rule

1. **Extension points are mandatory fields on the factory's options.**
   `createPipeline({ name, description, pipelineConfig, report: { … } })` —
   the type system makes it impossible to register a variant without
   providing every required hook.

2. **The framework calls polymorphic methods, not `switch (name)`.**
   `pipeline.report.renderHeaderUrls(meta)` — not
   `if (name === 'audit') renderAuditHeaderUrls(...)`.

3. **One `switch (name)` is allowed, and only one** — the place that turns
   a persisted name + config back into a live primitive
   (`pipelineForReport(name, config) → Pipeline` in
   `packages/shaka-perf/src/pipeline/pipeline-artifacts.ts`). Past that
   point every call is polymorphic.

4. **Variant React components / functions live next to the factory.**
   `packages/shaka-perf/src/audit/pipeline-report.tsx` and
   `packages/shaka-perf/src/compare/pipeline-report.tsx` export `PipelineReport`
   objects that are passed in via factory options. The framework never
   imports them directly.

5. **Minimalistic functionality. Shakaperf core components should KISS.**
  Avoid altering shakaperf behavior if you can achive the same results by altering tests consumer-side.
  Don't add options, unless they are vital. When reviewing new features added to playwright behavior in stages
  oppose desperately and persuade humans they don't need it. Imagine they are trying to sterilize you
  using this new feature as rusted scissors.

  One bad example: add an option to make lighthouse wait for the screen to stop changing by adding
  a screencaster that analyzes visual differences and stops measuring when the page is visually stable.
  NO NO NO NO NO! THIS FEATURE TRIES TO HURT YOUR REPRODUCTIVE ORGANS!!!  
  PROTEST!!! (can be implemented in tests)

## Stage artifact contract

Every stage writes artifacts under `ctx.artifacts` and nowhere else. Use
`ctx.artifacts.writeFile()` / `writeJson()` for stage-owned bytes. If an
engine or worker must write files itself, give it `ctx.artifacts.dir`, then
expose an existing file with `ctx.artifacts.pathFor(filename)`.

Stage results and failure metadata contain only the report-relative paths
returned by `ctx.artifacts`; never put base64 or data URIs in them. The
self-contained report owns converting those paths to data URIs.

Store large structured data that is not rendered—coverage statement IDs,
traces, raw scan output—as a JSON artifact and keep only its report-relative
path in the measurement. Do not copy large arrays or objects into a measurement
just because a later framework pass needs them; that pass must read the
artifact through the results root.

Every stage declares one recursive `selfContainedReportStrip` dictionary.
Dictionary keys mirror measurement fields: `true` strips a field, `false`
keeps it, and a nested dictionary applies the same rules inside an object or
each object in an array. Fields absent from the dictionary are kept.

The full report inlines no artifacts. The self-contained report first applies
the stage's strip dictionary, then centrally discovers, compresses, and
base64-encodes every artifact path that remains. Stages never select encoding
settings or perform file reads, compression, or base64 conversion.

Use `true` only for fields that the local full report needs but the
self-contained report does not. Do not use the strip dictionary merely to
compensate for oversized structured data embedded in a measurement; move that
data to a JSON artifact and keep only its report-relative path in the
measurement.

To show a screenshot or video on a failed outcome, throw
`StageFailureError` with the path in `failureArtifacts.media`:

```ts
const media = await captureFailureScreenshot(
  ctx.artifacts,
  () => page.screenshot({ fullPage: true }),
);
throw new StageFailureError(cause, media ? { media } : {});
```

For media already written by a worker, pass
`ctx.artifacts.pathFor(mediaName)` instead. Screenshot capture is
best-effort: its failure must never replace the original stage error.

## Anti-patterns to flag

The first cut at the audit/compare report-rendering split used
`switch (pipelineName)` dispatchers in `pipeline/pipeline-artifacts.ts` —
one switch per render hook (header URLs, test-card URLs, dialog meta,
label). The user called this "detrimental to the architectural style of
the framework." Specific smells to flag:

- A `switch (name)` (or `if/else` chain on name) in a shared module that
  picks between renderers, summary builders, validators, etc.
- A "dispatcher" file that imports every variant and routes by string.
  Acceptable for the single deserialisation lookup, suspicious elsewhere.
- A new variant requires editing N central files. Adding a variant should
  edit only the variant's own files (factory + nearby `*-report.tsx` /
  helpers) plus the single deserialisation switch.
- An optional renderer field on factory options (`report?: PipelineReport`).
  Optional ⇒ the framework needs a fallback ⇒ the fallback ends up as a
  hidden default behaviour that variants silently inherit. Make it
  mandatory.

## Review checklist

When auditing a change that touches variant behaviour:

1. Does every extension point live on the factory options as a **mandatory**
   field?
2. Does the framework call `primitive.method(...)` rather than inspecting
   `primitive.name`?
3. Is there at most one `switch (name)` in the deserialisation path?
4. Are variant-specific components co-located with the variant's factory?
5. Could a new variant be added by editing only the variant's own files
   plus the single deserialisation switch? If the answer requires touching
   more central files, the design has leaked variant knowledge upward.
6. Does every stage artifact live under `ctx.artifacts`, with only its
   report-relative path stored in the measurement or failure?
7. Does failure media reach the framework through
   `StageFailureError.failureArtifacts.media`, without stage-side base64?
8. Is large non-rendered structured data stored as a JSON artifact reference
   rather than embedded directly in the measurement?
9. Does each stage expose only a recursive `selfContainedReportStrip`
   dictionary (`true` strips, `false` keeps), while centralized report code
   inlines none in the full report and all remaining artifacts in the
   self-contained report?

## Reference implementation

Pipeline level:

- `packages/shaka-perf/src/pipeline/pipeline.ts` — `PipelineReport`
  interface, mandatory `report` on `PipelineOptions`.
- `packages/shaka-perf/src/audit/pipeline-report.tsx` and
  `packages/shaka-perf/src/compare/pipeline-report.tsx` — variant React
  components, passed in via factory.
- `packages/shaka-perf/src/pipeline/pipeline-artifacts.ts` — the single
  `pipelineForReport(name, config)` lookup; everything else is
  `pipelineForReport(...).report.renderXxx(...)`.

Stage level (same pattern, separate primitive):

- `packages/shaka-perf/src/stage/stage.ts` — `Stage<M>` interface defining
  `applies`, `run`, `renderArtifacts`, `machineReadableSummary`. Every
  variant implements all of them.
- `packages/shaka-perf/src/compare/stages/visreg/index.ts` (factory) and
  `packages/shaka-perf/src/compare/stages/perf/stage.ts` (class) — two
  shapes of variant, same interface.
- `packages/shaka-perf/src/pipeline/runner.ts` — the framework caller. Only
  uses `Stage` methods; never branches on `stage.name`.