diff --git a/.changeset/agent-review-config.md b/.changeset/agent-review-config.md new file mode 100644 index 00000000..a845151c --- /dev/null +++ b/.changeset/agent-review-config.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/.claude/.gitignore b/.claude/.gitignore new file mode 100644 index 00000000..cc1991b8 --- /dev/null +++ b/.claude/.gitignore @@ -0,0 +1,4 @@ +settings.local.json +worktrees/ +*.lock +.DS_Store diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md new file mode 100644 index 00000000..16d02df7 --- /dev/null +++ b/.claude/CLAUDE.md @@ -0,0 +1,93 @@ +# Project Guide + +Bruno API Docs fork — open-source API docs generated from a Bruno collection (React + Redux + +Vite). One workspace: `packages/bruno-api-docs`, published as `@opencollection/docs`. + +Collection docs render the collection a team already runs: every folder, request, and environment +gets a page (method, URL, params, headers, body, auth, examples, code snippets, scripts, tests), +and the embedded playground lets readers edit and send those requests from the docs. The output +is one static HTML file plus the CDN bundle this repo builds. + +The app is an API **client**, not a server. Judge every behaviour and edge case by "what should an +API client do here?", not what a document viewer or an API server would do. + +## Quick commands + +```bash +nvm use && npm install # Node from .nvmrc; installs the husky pre-commit hook +npm run lint # root +npm run lint:fix # root: auto-fix; the pre-commit hook runs this too +``` + +From `packages/bruno-api-docs/`: + +```bash +npm run dev # Vite on http://127.0.0.1:3001 (?fixture=folders|vars|descriptions|qa) +npm run test:run # Vitest one-shot (pretest builds the QuickJS lib bundle) +npm run test:run -- src/utils/cx.spec.ts # single unit spec +npm run test:e2e # Playwright; starts the dev server itself +npx playwright test e2e/tests/sidebar/ # one e2e directory +npm run build && npm run build:standalone # library + CDN bundle (dist/, dist-standalone/) +``` + +Prefer the smallest scope (one spec, one directory) over the full suite. + +## Key architecture + +- **Entry**: `src/components/OpenCollection/OpenCollection.tsx` owns the store, parses the + collection (YAML, then JSON fallback), and renders `AppShell` inside a `HashRouter`. + `components/PageRouter` maps routes (`src/routing/`) to `src/pages/*`. +- **Layers**: `src/pages/` routed screens, `src/components/` reusable UI, `src/ui/` primitives, + `src/hooks/`, `src/utils/` pure helpers, `src/store/` Redux Toolkit slices, `src/runner/` + request execution, `src/scripting/` the QuickJS sandbox and `bru.*` runtime, `src/theme/` + tokens. List a directory for the current set; do not trust a catalogue in a doc. +- **Standalone bundle**: `src/standalone.ts` (`OpenCollectionRenderer`) is what the HTML Bruno + generates loads from the CDN. Build output lives in `dist*/`; edit `src/` only. +- **Theming**: tokens in `src/theme/tokens/{light,dark}.ts` become CSS custom properties in + the generated `src/styles/theme.generated.css`. Components consume `var(--...)` only. + +## Coding standards + +Full list: `CODING_STANDARDS.md` (read it before writing code). Mechanical style is +ESLint-enforced; the rules worth holding in every session: + +- Colours and fonts only via CSS custom properties; hex literals fail lint outside `src/theme/`. +- Slices import through `@/store/slices/`; `@slices/*` fails lint. Other `src/` imports + use `@/*`. `e2e/` has no aliases. +- One component per folder (`Foo.tsx` + `StyledWrapper.ts` + `Foo.spec.tsx`); `testId` prop + with derived child ids; classes over inline `style`; no comments in `StyledWrapper.ts`. +- `description` fields are string **or** `{ content }`; always go through the normalisers. +- Every changed behaviour maps to a unit spec (via `useRenderToDom`) or an e2e spec. + +## Testing + +- **Unit**: Vitest, `environment: 'node'`, specs beside the code as `*.spec.ts(x)`. Render with + `useRenderToDom` and query with `src/test-utils/dom.ts`. No DOM interaction tests here. +- **E2E**: Playwright, class-based page-object model under `packages/bruno-api-docs/e2e/`. + Guide: `e2e/README.md`; quick reference: `.claude/rules/testing.md`; use `/write-e2e-test`. +- **CI** (`.github/workflows/ci.yml`): lint, unit tests, both builds, then e2e. Draft PRs skip + the builds and e2e. + +## Rules and skills + +Path-scoped rules in `.claude/rules/` attach when you touch matching files: `app-conventions` +(components, styling, state, format consumption), `conventions` (readability, comment and diff +hygiene), `cross-os-compat` (line endings, modifier keys, SSR safety), `unit-testing`, +`testing` (Playwright quick reference). Skills: `/code-review` (parallel lenses mirroring +`.coderabbit.yaml`), `/write-e2e-test`, `/new-component`. Layout and maintenance notes: +`.claude/README.md`. + +## Gotchas + +- `tsc` and Vitest fail with `Cannot find module './bundled-libraries.iife.js'` until + `npm run build:lib-bundle` has run once (`pretest`/`predev`/`prebuild` do it for you). +- Never edit `src/styles/theme.generated.css`; change the tokens and run `npm run gen:theme`. +- The dev entry `src/dev.tsx` mounts e2e fixture collections via `?fixture=`. Do not add ad-hoc + user collections there; exercise them through the standalone build instead. +- Every PR that changes published behaviour needs a changeset (`npm run changeset` or a + `changeset:patch|minor|major` label). Tooling-only PRs use an empty changeset. + +## Before you call a change done + +From the root: `npm run lint`. From `packages/bruno-api-docs/`: `npm run test:run`, plus +`npm run test:e2e` when UI behaviour changed. Remove dead code with the feature that used it. diff --git a/.claude/README.md b/.claude/README.md new file mode 100644 index 00000000..53f19fe8 --- /dev/null +++ b/.claude/README.md @@ -0,0 +1,115 @@ +# Claude Config + +The [Claude Code](https://code.claude.com/docs) configuration for Bruno API Docs: project +context, path-scoped engineering rules, and review/test skills. Claude picks it up automatically +when launched from the repo root. It is committed so every contributor and CI reviewer works +from the same conventions. + + + +## How the pieces link + +``` +CODING_STANDARDS.md (repo root) ← single source of truth for how code is written + ├── .coderabbit.yaml ingests it as review guidelines (knowledge_base) + ├── .claude/CLAUDE.md pointer + the few rules worth holding every session + ├── .claude/rules/*.md judgment layer + repo detail on top of it + └── .claude/skills/code-review/ local mirror of the CodeRabbit review +packages/bruno-api-docs/e2e/README.md ← canonical e2e guide; referenced by .coderabbit.yaml, + rules/testing.md and the write-e2e-test skill +``` + +Change a standard in `CODING_STANDARDS.md` first. The other files reference it; they should not +restate it. If two files disagree, the rules win over `.coderabbit.yaml`, and the standards file +wins over both. + +## What's inside + +| Path | What it is | Loads | +|------|------------|-------| +| `CLAUDE.md` | Project overview: commands, architecture pointers, standards summary, gotchas, index of rules and skills. | Every session. | +| `rules/app-conventions.md` | Components, styling, state, format consumption, test ids for `packages/bruno-api-docs/src/**`. | When Claude touches a matching file. | +| `rules/conventions.md` | Readability, comment and diff hygiene for all packages, scripts, examples. | On match. | +| `rules/cross-os-compat.md` | Line endings, modifier keys, SSR safety for `src/**`. | On match. | +| `rules/unit-testing.md` | Vitest: `useRenderToDom`, unconditional assertions, coverage mapping. | On `*.spec.*` / `*.test.*`. | +| `rules/testing.md` | Playwright quick reference for `e2e/**`. | On match. | +| `skills/code-review/` | `/code-review`: parallel lenses in `reviewers/`; mirrors `.coderabbit.yaml`. | On invocation or when relevant. | +| `skills/write-e2e-test/` | `/write-e2e-test`: a spec in the class-based page-object style. | On invocation or when relevant. | +| `skills/new-component/` | `/new-component`: scaffold a component or page with the folder, styling, and spec conventions. | On invocation or when relevant. | +| `settings.json` | Shared settings. Denies `Read` on build output and Playwright artefacts so Claude works from `src/`. | At startup, from the launch directory. | +| `settings.local.json` | Per-machine overrides. Gitignored. | At startup, if present. | + +## Install + +Start Claude Code (`claude`) from the repo root and everything loads: + +- `.claude/CLAUDE.md` is a first-class project-instruction location, so there is no root + `CLAUDE.md` and no `@` import. `CLAUDE.local.md` at the root is gitignored for personal notes. +- Path-scoped rules attach when Claude reads a matching file. Launching from + `packages/bruno-api-docs/` still loads this root `.claude/` from the ancestor directory. +- Skills are discovered from `.claude/skills/`: type `/code-review`, `/write-e2e-test`, + `/new-component`. + +Run `/context` in a session to confirm what loaded. + +--- + +## Maintaining this config + +For whoever edits the config. The goal is high instruction adherence at the lowest always-loaded +cost: put each instruction in the mechanism that loads it exactly when it is needed, and no +sooner. It follows the Claude Code docs; read them before structural changes: +[Write an effective CLAUDE.md](https://code.claude.com/docs/en/best-practices#write-an-effective-claude-md), +[Memory](https://code.claude.com/docs/en/memory) (loading order, `.claude/rules/`), +[Skills](https://code.claude.com/docs/en/skills). + +### Where does a new instruction go? + +| Mechanism | Lives in | Loads | Use it for | +|---|---|---|---| +| Coding standard | `CODING_STANDARDS.md` | Read on demand by Claude; ingested by CodeRabbit | Any rule about how code is written. Humans read it too. | +| Project instructions | `.claude/CLAUDE.md` | Every session (full file) | Facts true in nearly every session and not inferable from code: orientation, setup, global invariants, pointers. | +| Path-scoped rule | `.claude/rules/.md` with `paths:` | When Claude reads a matching file | Judgment calls and repo detail for one area. One topic per file. | +| Skill | `.claude/skills//SKILL.md` | On `/invoke` or when the description matches | A reusable multi-step procedure (review, scaffold, write a test). Not a fact. | +| Settings / hooks | `.claude/settings.json` | Startup / lifecycle events | Deterministic enforcement. Advisory guidance is not a hook. | +| CI review | `.coderabbit.yaml` | Every PR | Path scope, tone, and what the standards file cannot say. Never a restated standard. | + +### Budgets + +- `CLAUDE.md`: target ≤ 120 lines, hard cap 200. For every line ask "would removing this cause + Claude to make a recurring project-specific mistake?" If not, cut or relocate it. +- Rules: one topic each. Keep `paths:` accurate against real repo paths. +- Skills: `SKILL.md` under 150 lines; descriptions under ~200 characters, leading with the words + a triggering request would contain. +- Keep the skill catalogue small: names and descriptions cost discovery context even though + bodies load lazily. + +### Keep it consistent + +- Before writing a fact, `grep -rn "" .claude CODING_STANDARDS.md .coderabbit.yaml`. If it + already exists, point to it instead of repeating it. +- Verify every rule and example against the actual repo. Grep the source; do not assume. +- Do not hardcode volatile catalogues (component lists, slice names, fixture names beyond the + ones the dev entry hard-wires). Describe the category and say where to read the current set. +- Team-wide requirements belong in these committed files, not only in a contributor's auto + memory, which is machine-local. +- `Read`-deny rules are for build output, not dependencies. `node_modules/` is deliberately not + denied; reading a dependency's types is legitimate. + +### Validate a change + +- `git diff --check`; `python3 -c "import json;json.load(open('.claude/settings.json'))"`. +- Every rule has `paths:` (an unscoped rule loads in every session): + `grep -L "paths:" .claude/rules/*.md` prints nothing. +- Cross-references resolve: `grep -rno "[A-Za-z0-9_./-]*\.md" .claude CODING_STANDARDS.md`. +- Loading: `/context` in a session; open a file under `src/` and under `e2e/` and confirm the + right rule attaches. +- Skill triggering: phrase a request the skill should catch ("review my changes", "add an e2e + test for search") and confirm it is offered; phrase a near-miss and confirm it is not. + +### When to revisit + +After Claude repeats the same project-specific mistake, after a repo restructuring (packages +moved, build tooling changed), or after a Claude Code release that changes loading or skill +behaviour. Treat config edits like code: review them in PRs. diff --git a/.claude/rules/app-conventions.md b/.claude/rules/app-conventions.md new file mode 100644 index 00000000..d476bfb4 --- /dev/null +++ b/.claude/rules/app-conventions.md @@ -0,0 +1,68 @@ +--- +paths: + - "packages/bruno-api-docs/src/**" +--- + +# App Conventions + +`CODING_STANDARDS.md` is the source of truth for how components, styling, state, and tests are +written; read it. This file holds the judgment calls and repo-specific detail a linter cannot +make. Derived from the existing codebase: match it. + +## Components + +- Before adding a component, hook, or helper, search `src/ui/`, `src/components/`, `src/hooks/`, + and `src/utils/` by concept, not by the name you would have picked, and read the nearest + sibling that solves the same shape of problem. Reuse is usually a net deletion. +- Where an existing primitive is almost right, widen it rather than standing up a near-duplicate + next to it; two near-identical implementations diverge silently. +- `src/ui/` holds primitives with no knowledge of collections (tables, tabs, modals, editors). + `src/components/` holds collection-aware pieces. `src/pages/` holds routed screens composed + from both. A helper that only one component uses lives next to that component; a shared one + lives in `src/utils/` with its own spec. +- `useEffect` is used throughout the codebase and is not banned; still prefer derived state and + event handlers where they are genuinely simpler. An effect that only mirrors a prop into state + is a smell. + +## Styling + +- Legacy alias variables (`--text-primary`, `--border-color`, `--bg-secondary`, ...) in + `src/styles/index.css` map onto the generated `--oc-*` tokens. Prefer an existing alias; add + a new alias there rather than reaching for a raw `--oc-*` token in a component. +- Headings inside `.markdown-documentation`: keep `line-height` unitless or at least the font + size, or multi-line headings clip. +- Tailwind utilities appear alongside Emotion for layout (`flex`, spacing). That is fine; colour, + font, and border tokens still come from CSS variables in the wrapper. + +## Reading collections + +- `description` may be a bare string or a legacy `{ content, type }` object. Display and search + both go through `descriptionText` / `resolveDescription` (`utils/description.ts`), + `getDescription` (`utils/request.ts`), or `getItemDescription` (`utils/schemaHelpers.ts`). + A new description-bearing field follows the same handling. +- Requests come in several protocols (HTTP, GraphQL, gRPC, WebSocket). Check how + `components/PageRouter` and `utils/schemaHelpers.ts` discriminate them before adding a branch. +- Playground state is seeded from the docs collection; changes to one side must keep the other + consistent. Read `store/slices/playground.ts` alongside `store/slices/docs.ts`. + +## Test ids + +- Components take `testId?: string`; child ids derive from it (`${testId}-row`) and are omitted + when unset. A component reused in several sections gets a distinct `testId` per instance so + e2e locators stay unambiguous. +- If an e2e test needs an element with no stable id, add a `testId` to the component. Never + locate by styling class or text in a spec. + +## Hygiene + +- Do not strip explanatory JSDoc from non-obvious logic (for example the parsing rules in + `utils/pathParams.ts`) during a refactor; those comments are the contract. +- The `@/*` alias is configured in `tsconfig.json`, `vite.config.ts`, `vite.config.*.ts`, and + `vitest.config.ts`. Keep them in sync if you touch one. + +## Before you call a change done + +From the root: `npm run lint`. From `packages/bruno-api-docs/`: `npm run test:run`, plus +`npm run test:e2e` when UI behaviour changed. Then list every behaviour the diff adds or changes +and name the test that exercises it. Anything without a test is a gap to fill before the commit, +not a note for the PR. diff --git a/.claude/rules/conventions.md b/.claude/rules/conventions.md new file mode 100644 index 00000000..7475a1e0 --- /dev/null +++ b/.claude/rules/conventions.md @@ -0,0 +1,57 @@ +--- +paths: + - "packages/**/*" + - "scripts/**/*" + - "examples/**/*" +--- + +# Readability and Diff Hygiene + +`CODING_STANDARDS.md` is the source of truth for coding standards; read it. This file is the +judgment layer: the readability and hygiene calls a linter cannot make. Code and comments must +read as a natural, permanent part of the project, never as artefacts of the task or session +that produced them. + +## Style and formatting + +Mechanical style (indent, quotes, semicolons, trailing commas, arrow parens, brace style, line +length) is ESLint-enforced and auto-fixed by `npm run lint:fix`. Note these deviations briefly +rather than dwelling on them. Naming and casing that ESLint cannot repair still warrant attention. + +## Readability + +- **Names say what they hold.** Concrete subject and type, understandable on first read. Raise an + unclear or misleading name even when the code is otherwise correct. +- **Reuse before you write.** Search for the existing component, hook, or helper by concept, then + read the nearest sibling solving the same shape of problem; its call site shows the intended + composition. +- **Extraction and abstraction.** Extract when it improves readability or serves a clear, + anticipated reuse; this is not gated on a minimum number of call sites. Avoid only indirection + that earns nothing: a utility generalised for one site with no foreseeable second user, or + options added "for later". +- **Single-line indirection.** A one-line function that only forwards to another should be inlined. +- **Optional chaining and falsy defaults.** `?.` only where the null case is handled right there. + `x || default` only where an empty string, `0`, or `false` genuinely means "unset". +- **Functional, but readable.** Obvious, linear pipelines over deep functional machinery. + +## Comments + +- **No situational comments.** Nothing that references the change, the task, or the review + (`// added to fix ...`, `// as requested`, `// per review`). State a reason as a timeless fact + about the code or link the issue. +- **No obvious comments.** Do not restate the code. If it is self-explanatory, leave it bare. +- **Comment the why.** Non-obvious rationale, invariants, edge cases, a workaround and the + constraint forcing it, units, a pointer to a spec. +- **No scaffolding or narration.** No `// ... existing code ...`, no TODO-for-me notes, no + commented-out code, no step-by-step change log in comments. +- **No comments in `StyledWrapper.ts` files.** + +## Beyond comments + +- **Anything added needs a live consumer in the same change.** No option nobody passes, payload + field nobody reads, or branch for a state the producer cannot emit. +- **Replacing code leaves nothing behind.** Removing a view or feature also removes its orphaned + components, props, store wiring, styles, and tests. Confirm what the new code actually renders + before calling a leftover dead. +- **Minimal diffs.** No unrelated reformatting or whitespace churn. +- **No ticket identifiers** in source, comments, or test names. diff --git a/.claude/rules/cross-os-compat.md b/.claude/rules/cross-os-compat.md new file mode 100644 index 00000000..087d32f1 --- /dev/null +++ b/.claude/rules/cross-os-compat.md @@ -0,0 +1,43 @@ +--- +paths: + - "packages/bruno-api-docs/src/**" +--- + +# Cross-OS Browser Compatibility + +The app runs in the reader's browser on macOS, Windows, and Linux, renders to static markup in +unit tests, and ships as a standalone bundle. The reader's OS changes what they type or paste and +which modifier key they press, so the same code can behave differently per platform. Handle these +explicitly. (This is about browser behaviour across the reader's OS, not desktop packaging.) + +## Line endings (CRLF vs LF) + +- Text from a Windows reader arrives with `\r\n`; macOS, Linux, and the Monaco editor default to + `\n`. When you split multiline text to process it (bulk editors, parsers), split on `/\r?\n/`, + never `'\n'`; a bare `'\n'` split leaves a trailing `\r` on every line. See + `utils/bulkKeyValue.ts`. +- `.trim()` on each field happens to mask a stray `\r`; do not rely on it. Normalise at the split + so the `\r` never enters the pipeline (it also breaks prefix checks like `startsWith('//')`). +- Pure line counting (`code.split('\n').length`) gives the same answer for CRLF, so it is not a + bug where it exists, but prefer `/\r?\n/` for consistency. +- Emit multiline text joined with `\n` as the canonical in-memory form. + +## Keyboard shortcuts and modifier keys + +- The primary modifier is `event.metaKey` on macOS and `event.ctrlKey` on Windows/Linux. A + shortcut gated on only one is dead on the other. Accept `event.metaKey || event.ctrlKey`. +- Detect the platform with `isMacPlatform()` from `utils/platform.ts` (SSR-safe, reads + `navigator.platform` with a `userAgent` fallback). Do not hand-roll the check or hardcode `⌘` + / `Ctrl` in shortcut hints; derive the label. +- Bare keys (Escape, arrows, Home/End, Enter) are cross-platform. See `ui/Tabs`, `ui/Modal`, + `ui/Dropdown` for the pattern. + +## SSR and environment safety + +- Unit specs render components through `useRenderToDom` (`src/hooks/useRenderToDom.ts`), which + runs a server render in Vitest's `node` environment: `window`, `document`, and `navigator` are + undefined. Never read them at module top level or during render; do it inside an effect or a + guarded helper (`typeof window !== 'undefined'`). +- Storage reads go through `useStorage` / `useLocalStorage` / `useSessionStorage` (`src/hooks/`), + which already handle the missing-storage case. +- No Node-only APIs (`fs`, `path`, `process`, `Buffer`) in runtime code under `src/`. diff --git a/.claude/rules/testing.md b/.claude/rules/testing.md new file mode 100644 index 00000000..268ac82c --- /dev/null +++ b/.claude/rules/testing.md @@ -0,0 +1,81 @@ +--- +paths: + - "packages/bruno-api-docs/e2e/**" + - "packages/bruno-api-docs/playwright.config.ts" + - "packages/bruno-api-docs/playwright-report/**" + - "packages/bruno-api-docs/test-results/**" +--- + +# Playwright E2E Quick Reference + +The narrative source of truth is `packages/bruno-api-docs/e2e/README.md`; read it for the full +walkthrough. Tests use a **class-based page-object model**: page objects and components are +classes handed to specs via fixtures, not a functional locator/action pattern. + +## Running + +```bash +npm run test:e2e # all specs, headless; starts the dev server itself +npm run test:e2e:ui # Playwright UI +npx playwright test e2e/tests/playground/ # one directory +npx playwright test --headed # watch +npx playwright test --debug # step debugger +``` + +From `packages/bruno-api-docs/`. Config: `playwright.config.ts`: `testDir: ./e2e`, one +`chromium` project, `fullyParallel: true`, retries 0 local / 2 CI, one worker in CI, +`trace: 'on-first-retry'`. `webServer` runs `npm run dev` on `http://127.0.0.1:3001`; never +start it by hand. CI runs the suite after lint, unit tests, and both builds pass. + +## The three building blocks + +Everything lives under `packages/bruno-api-docs/e2e/`: + +- **Page objects** (`pages/*.page.ts`) describe a whole screen. Each extends `BasePage` (owns + `goto`/`reload` and a `root` locator), sets its own `root`, and composes the components a test + cares about as readonly fields. Screens with no URL of their own expose `open(path: string[])` + that navigates the sidebar and waits for `root`. +- **Components** (`components/*.component.ts`) are reusable pieces. Each extends + `BaseComponent` (every component has a `root: Locator`). Common controls (sidebar, markdown, + tooltip) live at the top level; sections that belong to a single page live in a subfolder + named after it (`components/overview/`, `components/playground/`, ...). A component derives + its child locators from `root` or a `testId` base. +- **Fixtures** (`playwright/pages.fixture.ts`, `playwright/digest-mock.fixture.ts`) instantiate + page objects and components. `playwright/index.ts` merges them with `mergeTests` and + re-exports `test`/`expect`, the single import for every spec. + +## Fixture collections + +The dev entry mounts a collection per `?fixture=` value (`folders`, `vars`, `descriptions`, +`qa`) from `src/e2eFixtures/`; no query mounts the sample collection. A spec picks its +collection by navigating to `/?fixture=` (see `e2e/tests/sidebar/sidebar.spec.ts`). Add a +new fixture there only when no existing one covers the shape you need, and name it after the +shape, not the ticket. + +## Locating elements + +- Locate by `data-testid` through page objects and components, never by styling class, tag, or + index. Derive child ids from a base (`getByTestId(\`${testId}-text\`)`), see + `components/secret-value.component.ts`. +- Rendered-Markdown internals have no test id; match them by role within their test-id'd + container (scope a `MarkdownComponent` to its `root`). +- Stable `getByRole` / accessible-name assertions are fine. If no stable selector exists, add a + `testId` to the component rather than locating by text. + +## Writing a spec + +- `import { test, expect } from '../../playwright';`. All imports in `e2e/` are relative; there + are no path aliases. +- Pull page objects and components off the test callback; no `new` in specs. +- Keep every `expect` in the spec. Page objects and components expose elements and actions only. +- Auto-retrying assertions (`await expect(locator).toBeVisible()`); never assert immediately after + navigation. Reserve `page.waitForTimeout()` for when no locator assertion can wait. +- Titles read like documentation: what the page does, in plain English. +- Fully parallel: no state shared between specs. + +## Common pitfalls + +1. Collapsed folders remove their children from the DOM; expand via the sidebar before asserting. +2. A component reused in several sections needs a distinct `testId` base per instance. +3. A raw `page.locator('.foo')` in a spec is a smell; extend a page object or component. +4. `test.only` fails CI (`forbidOnly`); `page.pause()` never ships. diff --git a/.claude/rules/unit-testing.md b/.claude/rules/unit-testing.md new file mode 100644 index 00000000..d610f227 --- /dev/null +++ b/.claude/rules/unit-testing.md @@ -0,0 +1,60 @@ +--- +paths: + - "packages/bruno-api-docs/src/**/*.spec.ts" + - "packages/bruno-api-docs/src/**/*.spec.tsx" + - "packages/bruno-api-docs/src/**/*.test.ts" + - "packages/bruno-api-docs/src/**/*.test.tsx" + - "packages/bruno-api-docs/src/test-utils/**" + - "packages/bruno-api-docs/vitest.config.ts" +--- + +# Unit Tests (Vitest) + +`CODING_STANDARDS.md` §Tests is the source of truth; this is the quick reference. + +## Running + +```bash +npm run test:run # all specs, one-shot (pretest builds the lib bundle) +npm run test:run -- src/utils/cx.spec.ts # one spec +npm test # watch mode +``` + +From `packages/bruno-api-docs/`. Config: `vitest.config.ts`, `environment: 'node'`, specs are +`src/**/*.{spec,test}.{ts,tsx}`; `e2e/**` is excluded. The husky pre-commit hook runs the staged +specs. + +## Rendering components + +Render through `useRenderToDom` and query with `src/test-utils/dom.ts`: + +```tsx +import { useRenderToDom } from '@/hooks/useRenderToDom'; +import { getByTestId, queryByTestId, query } from '@/test-utils/dom'; + +const root = useRenderToDom(); +expect(getByTestId(root, 'example-tab-a').getAttribute('aria-selected')).toBe('true'); +expect(queryByTestId(root, 'example-tab-b-panel')).toBeNull(); +``` + +- `getByTestId` / `query` throw when the element is absent, so a missing element fails the test + with a clear message. Use `queryByTestId` only when asserting absence. +- Older specs assert on raw `renderToStaticMarkup` strings. Leave them until touched; new specs + use `useRenderToDom`. +- There is no DOM event model here. Clicks, typing, focus, and scroll are Playwright territory. +- Storage-dependent code takes the in-memory `fakeStorage()` from `src/test-utils/storage.ts`. + +## Assertions + +- Unconditional. No `?.`, `??`, `||`, `if`, or try/catch around or inside an assertion; a guard + that lets the assertion be skipped turns a failure into a silent pass. +- Assert the unique value the change under test produces, not a substring the fixture already + carries elsewhere. +- Cover the happy path and the realistic failure paths (malformed collection field, missing + description, disabled header, empty auth) as an API client would meet them. + +## Coverage mapping + +For each behaviour the diff adds or changes (new branch, default, UI state, bug fix), name the +spec that exercises it. A bug fix ships with a regression test. Pure helpers in `src/utils/`, +`src/runner/`, `src/scripting/`, and `src/routing/` get a spec file beside them. diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 00000000..ec5fabd3 --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,10 @@ +{ + "permissions": { + "deny": [ + "Read(./**/dist/**)", + "Read(./**/dist-standalone/**)", + "Read(./**/playwright-report/**)", + "Read(./**/test-results/**)" + ] + } +} diff --git a/.claude/skills/code-review/SKILL.md b/.claude/skills/code-review/SKILL.md new file mode 100644 index 00000000..bf8bd060 --- /dev/null +++ b/.claude/skills/code-review/SKILL.md @@ -0,0 +1,74 @@ +--- +name: code-review +description: Review a Bruno API Docs diff, PR, or branch via focused reviewers run in parallel — + correctness, conventions, React/styling, security, unit tests, e2e tests. Mirrors the + CodeRabbit / CODING_STANDARDS review. +--- + +# Reviewing Bruno API Docs code + +This skill mirrors the automated CodeRabbit review (`.coderabbit.yaml`) so you can run the same +review locally. Source-of-truth order: coding standards → `CODING_STANDARDS.md`; app behaviour and +conventions → `.claude/rules/*`. `.coderabbit.yaml` mirrors these for CI parity; read it for a +path instruction not summarised here, but the rules win if they ever disagree. + +The review is **split into focused lenses that run in parallel**, so a large diff is covered +faster and each reviewer stays sharply scoped to one concern. Each lens lives in its own file +under `reviewers/`. You (the orchestrator) dispatch the reviewers, then merge and report. + +## How to review (orchestration) + +1. **Get the diff.** Review only what changed, never untouched code. Pick the mode: + - **Committed range** (default): `git diff main...HEAD` against the base branch (`main` or + `release/*`). Update the base first (`git fetch` and confirm the local base is current); a + stale base inflates the diff with already-merged changes and wastes a full fan-out. Each + reviewer re-runs this scoped to its own globs; the range is pinned to fixed commits so they + all see identical bytes. + - **Working tree / uncommitted changes**: the tree can shift mid-review, so capture the diff + **once** to a scratch file and hand every reviewer that path. `git diff HEAD` covers staged + and unstaged tracked changes; run `git add -N .` first (reversible with `git reset`) so new + untracked files show up: + ```bash + git add -N . && git diff HEAD > "$SCRATCH/review.diff" + ``` + `$SCRATCH` is your environment's scratchpad dir. Reviewers read this frozen diff for their + globs plus the on-disk files for surrounding context. +2. **Enumerate changed files**: `git diff --name-only main...HEAD` (committed range) or + `git diff --name-only HEAD` (working tree). Skip any lens whose file scope is not touched (no + `packages/bruno-api-docs/e2e/**` change → skip `e2e-tests.md`; no `*.spec.*` change → skip + `unit-tests.md`). Never skip the lenses scoped to all files. +3. **Fan out the reviewers in parallel.** In a *single message*, launch one `Agent` + (subagent_type `Explore` or `general-purpose`) per in-scope reviewer below. Give each subagent + this exact briefing: + - The diff source (the committed range, e.g. `main...HEAD`, or the snapshot path + `$SCRATCH/review.diff`) and the file globs it owns (from the reviewer file's "Scope" line). + For a snapshot, tell the reviewer to read that file for its globs rather than re-run + `git diff`. + - "Read `.claude/skills/code-review/reviewers/_contract.md` for the shared persona and output + contract, then read `.claude/skills/code-review/reviewers/` and any rule or source file + it points to (`CODING_STANDARDS.md`, `.claude/rules/*.md`), which hold the detailed + checklist. Apply **only** that lens to the changed files in your scope, at the severities the + reviewer specifies. Do not review outside your scope." + Reviewers are read-only and independent; overlap between lenses is fine (you dedupe at merge). +4. **Merge and report.** Collect every reviewer's findings, drop exact duplicates, and when two + lenses flag the same `file:line` keep the higher severity. Regroup **by file**, each finding + tagged by severity (blocker / suggestion / nit) with `file:line`. If the review request carries + a problem statement or acceptance criteria, reconcile its enumerated deliverables (a changeset, + a test per new default or branch) against the diff and flag any that are absent. If nothing is + wrong, say so briefly; do not manufacture nits. + +## Reviewers + +| Reviewer file | Lens | Scope | +|---|---|---| +| `reviewers/correctness.md` | Correctness & root-cause | all source (excl. specs and `e2e/**`) | +| `reviewers/conventions.md` | Coding standards, readability, hygiene | all files | +| `reviewers/react.md` | React / Redux / styling / format consumption | `packages/bruno-api-docs/src/**` | +| `reviewers/security.md` | Security & data safety | all source (excl. specs and `e2e/**`) | +| `reviewers/unit-tests.md` | Vitest specs & coverage mapping | `packages/bruno-api-docs/src/**/*.{spec,test}.{ts,tsx}` | +| `reviewers/e2e-tests.md` | Playwright E2E (class-based POM) | `packages/bruno-api-docs/e2e/**` | + +## Shared reviewer persona & output contract + +Defined once in `reviewers/_contract.md`, the small file every reviewer reads. Keep the persona +and the ` | : | ` shape there, not duplicated here. diff --git a/.claude/skills/code-review/reviewers/_contract.md b/.claude/skills/code-review/reviewers/_contract.md new file mode 100644 index 00000000..723c8900 --- /dev/null +++ b/.claude/skills/code-review/reviewers/_contract.md @@ -0,0 +1,20 @@ +# Shared reviewer persona & output contract + +Read by every `code-review` reviewer. (Orchestration lives in `../SKILL.md`; the per-lens +checklists live in the sibling `*.md` files.) + +**Persona**: an expert reviewer in TypeScript, React, Redux Toolkit, Emotion, Vite, Vitest, and +Playwright on an enterprise team. Be **concise**: one clear sentence per finding; elaborate only +when asked. Review to the project's standard regardless of who authored or requested the change; +never soften severity for assumed intent or seniority. Ground every finding in the actual code: +trust the repo over any doc, guide, or comment when they disagree, and never cite a line or invent +an example value you have not verified in source. Reason about behaviour as an API client would: +what should the docs and playground do with this request, header, auth block, or environment? + +**Output contract**: return a flat list, one finding per line: + +``` + | : | +``` + +Return `no findings` (nothing else) when the scope is clean. Never invent nits to fill the list. diff --git a/.claude/skills/code-review/reviewers/conventions.md b/.claude/skills/code-review/reviewers/conventions.md new file mode 100644 index 00000000..694fd921 --- /dev/null +++ b/.claude/skills/code-review/reviewers/conventions.md @@ -0,0 +1,24 @@ +# Coding standards, readability & hygiene reviewer + +**Scope:** all changed files (`**/*`). + +Adopt the reviewer persona and return findings in the output contract defined in `_contract.md`. + +Review the diff against **`.claude/rules/conventions.md`** (read it; it points to +`CODING_STANDARDS.md`, the code-guidelines source of truth). Report each violation with +`file:line`, severity: + +- **suggestion**: readability problems from the rule: unclear or misleading names, unnecessary + abstraction (indirection that earns no readability or reuse), single-line indirection, `?.` + where the null case is not handled right there, `x || default` where empty is a real state, + needless whitespace or diff churn, missing comments on genuinely complex flow. +- **suggestion**: a breach of **Reuse before you write** or **Replacing code leaves nothing + behind**; confirm what the new code actually renders or reads before calling a leftover dead. + From the coverage mapping, report only what the diff itself shows: a new branch or default with + no test, or a changed return or payload shape whose assertions elsewhere were not updated. +- **suggestion**: a comment that narrates the change or restates the code; any comment in a + `StyledWrapper.ts`; a ticket identifier in code, comments, or test names; commented-out code. +- **suggestion**: a cross-tree relative import (`../../utils/x`) where the `@/*` alias covers the + target; any `@slices/*` import; a runtime import of a `devDependencies` package. +- **nit**: pure style deviations (indent, quotes, semicolons, trailing commas, arrow parens, + casing, line length); ESLint auto-fixes most of these, so keep them brief. diff --git a/.claude/skills/code-review/reviewers/correctness.md b/.claude/skills/code-review/reviewers/correctness.md new file mode 100644 index 00000000..c2310757 --- /dev/null +++ b/.claude/skills/code-review/reviewers/correctness.md @@ -0,0 +1,30 @@ +# Correctness & root-cause reviewer + +**Scope:** all changed source (`packages/**`, `scripts/**`), excluding `*.spec.*`, `*.test.*`, +and `packages/bruno-api-docs/e2e/**`. + +Adopt the reviewer persona and return findings in the output contract defined in `_contract.md`. + +Validate that the change is correct and, for every bug fix, that it addresses the underlying +problem rather than the visible symptom. Surface-level patches that mask a defect without +resolving it are a **blocker**: + +- Understand *why* the issue exists before accepting the fix; trace the bug to its origin. +- Flag patches that suppress a symptom (extra null guards, try/catch swallowing, defensive + re-checks, retries, timeouts, clamping) while leaving the real cause in place. If this defends + against a bad value, where does the bad value come from, and should it be fixed there? +- A fix at the wrong layer (a UI guard for a data-layer bug, a caller working around a callee's + contract violation) is a symptom patch; name the correct layer. +- A fix scoped to one reproduction when the same root cause can manifest elsewhere; the correct + fix usually covers all call sites. +- When a fix looks like a workaround, say so and name the deeper change that would resolve it. +- Ordinary correctness: off-by-one and boundary errors, unhandled promise rejections and missing + `await`, swallowed errors, wrong null/undefined handling, edge cases the change introduces. +- **`x || default` on a field whose absence is meaningful.** For an API client, a deliberately + empty auth token, param, or header is a real state. Falsy-coalescing fabricates a value and + erases the distinction; use `??` or an explicit `undefined` check. +- **Twin paths.** Docs pages and the playground render the same collection from separate slices + (`store/slices/docs.ts`, `store/slices/playground.ts`). A change that touches how one reads a + field (description, auth, params, body) must be checked against the other; divergence is a bug. +- **Format unions.** `description` is string or `{ content }`; request shapes differ per protocol. + Direct property access that assumes one shape is a **blocker** when the other shape reaches it. diff --git a/.claude/skills/code-review/reviewers/e2e-tests.md b/.claude/skills/code-review/reviewers/e2e-tests.md new file mode 100644 index 00000000..2fe27cd3 --- /dev/null +++ b/.claude/skills/code-review/reviewers/e2e-tests.md @@ -0,0 +1,24 @@ +# E2E tests reviewer + +**Scope:** files under `packages/bruno-api-docs/e2e/**` and `packages/bruno-api-docs/playwright.config.ts`. + +Adopt the reviewer persona and return findings in the output contract defined in `_contract.md`. + +Review the diff against **`.claude/rules/testing.md`** and the class-based page-object model +described in `packages/bruno-api-docs/e2e/README.md` (read both). Report violations with +`file:line`, severity: + +- **blocker**: `test.only`; `page.pause()`. +- **suggestion**: a raw selector inlined in a spec (`page.locator('.foo')`) instead of a page + object or component field; locating by styling class, tag, or index where a `data-testid` + exists or should be added; a new page object that does not extend `BasePage`, or a component + that does not extend `BaseComponent`; a page object or component added without a fixture in + `e2e/playwright/` (specs get it off the test callback, never `new` it); an `expect` hidden + inside a page object; `page.waitForTimeout()` where an `expect()` locator assertion could wait; + a path alias in `e2e/` (imports there are relative); a non-discriminating assertion that passes + even if the change under test never happened. +- **suggestion**: a user-visible behaviour the wider diff adds or changes with no e2e spec covering + it (read the non-test files in the diff to judge); a spec relying on state left by another spec. +- **nit**: a reused section component given a non-unique `testId` base; a single broad assertion + where several focused ones fit; a page-specific section not placed in its page-named subfolder + under `components/`; a title that does not read like documentation. diff --git a/.claude/skills/code-review/reviewers/react.md b/.claude/skills/code-review/reviewers/react.md new file mode 100644 index 00000000..49d891aa --- /dev/null +++ b/.claude/skills/code-review/reviewers/react.md @@ -0,0 +1,34 @@ +# React / Redux / styling reviewer + +**Scope:** `packages/bruno-api-docs/src/**` (excluding `*.spec.*` and `*.test.*`). + +Adopt the reviewer persona and return findings in the output contract defined in `_contract.md`. + +Review changed components against **`CODING_STANDARDS.md`** §React components, §Styling and +theming, §State, §Reading the OpenCollection format, and **`.claude/rules/app-conventions.md`** +(read both). Report violations with `file:line`, severity: + +- **blocker**: a hardcoded hex/rgb/hsl/named colour or font family instead of a CSS custom + property (breaks light/dark theming; ESLint catches hex only); a `.description` read that + assumes a bare string or a `{ content }` object without going through the normalisers; store + access that bypasses `useAppSelector` / `useAppDispatch` from `src/store/hooks`; a component that + mixes controlled and uncontrolled state; a hook called after a conditional early return; a + namespaced hook import (`React.useState`); an edit to `src/styles/theme.generated.css`. +- **blocker**: a removed render path that leaves behind unused components, props, a no-op effect, + or store state that is set but never read. +- **suggestion**: a static inline `style={{ ... }}` that belongs on a className in the component's + `StyledWrapper.ts` (inline is for runtime-computed values only); Tailwind used for colour or + font rather than layout; a `useEffect` that only mirrors a prop into state or could be a derived + value or event handler; a missing memo that breaks a dependency array, or a gratuitous memo on + a cheap primitive; a `window`/`document` read at module scope or during render. +- **suggestion**: an e2e-targetable element without a `testId` prop / `data-testid`; a child + `data-testid` not derived from the base `testId`, or not omitted when `testId` is unset; a + reused component given a non-unique `testId`; an interactive element missing `aria-label`, + `aria-pressed`, or `type="button"`, or a decorative icon missing `aria-hidden="true"`. +- **suggestion**: a component placed in the wrong layer (`src/ui/` primitive that knows about + collections, `src/components/` piece that duplicates a `src/ui/` primitive); a monolithic + component that should compose `Section`, `Heading`, `EmptyState`, `Tabs`, and friends; a + near-duplicate of an existing hook or helper. +- **suggestion**: optimistic success state (`copied`, `saved`) set unconditionally rather than + gated on the operation resolving, for example after an optional-chained + `navigator.clipboard?.writeText`. diff --git a/.claude/skills/code-review/reviewers/security.md b/.claude/skills/code-review/reviewers/security.md new file mode 100644 index 00000000..246e3eb1 --- /dev/null +++ b/.claude/skills/code-review/reviewers/security.md @@ -0,0 +1,33 @@ +# Security & data safety reviewer + +**Scope:** all changed source (`packages/**`, `scripts/**`), excluding `*.spec.*`, `*.test.*`, +and `packages/bruno-api-docs/e2e/**`. + +Adopt the reviewer persona and return findings in the output contract defined in `_contract.md`. + +The app renders user-authored collections (Markdown, request definitions, secret and environment +values) into a static docs site, runs reader-editable scripts in a QuickJS sandbox, and sends +real requests from the reader's browser. Review changes for these risks: + +- **No secret leakage.** Auth tokens, passwords, API keys, OAuth2 secrets, and environment values + from a collection must never reach logs, console, error messages, generated code snippets, or + the URL. Masked values (`ui/SecretValue`) stay masked until explicitly revealed. A secret logged + or rendered in cleartext is a **blocker**. Watch for calls that dump a whole request, header + set, environment, or store slice. +- **XSS via rendered content.** Collection Markdown, descriptions, names, example bodies, and + script sources are untrusted. Markdown renders through `hooks/useMarkdownRenderer` with + `html: false`; a new render path that bypasses it, enables raw HTML, or adds + `dangerouslySetInnerHTML` on collection data is a **blocker**. Response bodies previewed as + HTML or SVG must stay sandboxed. +- **Sandbox integrity** (`src/scripting/`). Widening what a script can reach (new host globals, + DOM access, fetch to arbitrary origins, storage) or passing unsanitised script output back into + privileged code is a potential escape; call it out. +- **Request construction** (`src/runner/`). Variable interpolation into URLs, headers, and bodies + must not let a value break out of its field (header injection via `\r\n`, URL scheme changes). + Digest and other auth computations must not log intermediate secrets. +- **Injection & unsafe eval.** String-built code, `eval`, dynamic `Function`, or `RegExp` built + from collection data. +- **Dependency & network surface.** New runtime dependencies or outbound calls a static docs + renderer should not need; the docs must render a collection without phoning home. + +Keep findings concrete: tie each to how the tainted value reaches the sink. diff --git a/.claude/skills/code-review/reviewers/unit-tests.md b/.claude/skills/code-review/reviewers/unit-tests.md new file mode 100644 index 00000000..bddbf3a1 --- /dev/null +++ b/.claude/skills/code-review/reviewers/unit-tests.md @@ -0,0 +1,22 @@ +# Unit tests reviewer + +**Scope:** `packages/bruno-api-docs/src/**/*.{spec,test}.{ts,tsx}` and +`packages/bruno-api-docs/src/test-utils/**`. + +Adopt the reviewer persona and return findings in the output contract defined in `_contract.md`. + +Review the diff against **`.claude/rules/unit-testing.md`** and `CODING_STANDARDS.md` §Tests +(read both). Also read the non-test files in the same diff so you can map behaviours to specs. +Report with `file:line`, severity: + +- **blocker**: a conditional assertion (`?.`, `??`, `||`, `if`, or try/catch that lets an + `expect` be skipped or an error be swallowed); a bug fix in the diff with no regression test. +- **suggestion**: a behaviour the diff adds or changes (new branch, default, UI state) with no spec + exercising it; name the behaviour and the kind of test missing. A green suite is not evidence + of coverage; the mapping is. +- **suggestion**: a new component spec asserting on raw `renderToStaticMarkup` strings instead of + `useRenderToDom` + `src/test-utils/dom.ts`; a DOM-interaction test (click, type, focus) that + belongs in Playwright; a non-discriminating assertion that passes even if the change never + happened; a test that only mirrors the implementation instead of observable output. +- **nit**: a test name that does not describe the behaviour; copy-pasted setup where an existing + helper or `test-utils` function fits; a ticket identifier in a test name. diff --git a/.claude/skills/new-component/SKILL.md b/.claude/skills/new-component/SKILL.md new file mode 100644 index 00000000..b5c88f21 --- /dev/null +++ b/.claude/skills/new-component/SKILL.md @@ -0,0 +1,108 @@ +--- +name: new-component +description: Scaffold a new React component or page in packages/bruno-api-docs following the + repo's folder, styling, test-id, and unit-test conventions. Use when adding UI to the docs + renderer or playground. +--- + +# Scaffold a component + +Create a component in `packages/bruno-api-docs` that matches `CODING_STANDARDS.md` §React +components, §Styling and theming, §Tests, and `.claude/rules/app-conventions.md`. Do not deviate +from these patterns. + +## Inputs + +- **Name** (PascalCase, e.g. `EnvironmentBadge`). +- **Layer**: primitive with no collection knowledge → `src/ui//`; collection-aware reusable + piece → `src/components//`; routed screen → `src/pages//`. +- **Needs a test id?** Almost always yes for anything e2e might target. + +## Before scaffolding + +Search `src/ui/`, `src/components/`, and `src/hooks/` by concept for an existing piece that +already does this; widening it beats a near-duplicate. Read the nearest sibling for composition. + +## Steps + +1. Create the folder and the three files below (plus `index.ts` only for pages or public entry + points). Wire real prop types from `@opencollection/types/...`; avoid `any`. +2. Style via Emotion in `StyledWrapper.ts` using CSS custom properties only. No hex literals, no + comments in the wrapper, no static inline `style`. +3. Run `npm run lint` (root) and `npm run test:run` (package) and fix anything. + +### `.tsx` + +```tsx +import React from 'react'; +import { StyledWrapper } from './StyledWrapper'; + +interface Props { + testId?: string; + className?: string; +} + +export const : React.FC<Props> = ({ testId, className }) => ( + + + +); + +export default ; +``` + +### `StyledWrapper.ts` + +```ts +import styled from '@emotion/styled'; + +export const StyledWrapper = styled.div` + color: var(--text-primary); + + .-value { + color: var(--text-secondary); + } +`; +``` + +### `.spec.tsx` + +```tsx +import React from 'react'; +import { describe, it, expect } from 'vitest'; +import { } from './'; +import { useRenderToDom } from '@/hooks/useRenderToDom'; +import { getByTestId, queryByTestId } from '@/test-utils/dom'; + +describe('', () => { + it('renders the root and derived child test ids', () => { + const root = useRenderToDom(< testId="x" />); + expect(getByTestId(root, 'x-value')).toBeTruthy(); + }); + + it('omits test ids when none is given', () => { + const root = useRenderToDom(< />); + expect(queryByTestId(root, 'x-value')).toBeNull(); + }); +}); +``` + +Vitest runs in `environment: 'node'`: assert on rendered text, `aria-*`, and `data-testid` +presence. Cover clicks and toggles with Playwright, not here. + +### `index.ts` (pages and public entry points only) + +```ts +export { } from './'; +export { default } from './'; +``` + +## Reminders + +- Interactive elements: `type="button"`, `aria-label`, `aria-pressed` for toggles; decorative + icons `aria-hidden="true"`. See `ui/SecretValue` and `ui/CopyButton`. +- Store access via `useAppSelector` / `useAppDispatch` from `@/store/hooks`; slices via + `@/store/slices/`. +- Keep test ids unique per instance when the component is reused across sections. +- Add an e2e component or page object under `e2e/` if the new UI is user-visible + (`/write-e2e-test`). diff --git a/.claude/skills/write-e2e-test/SKILL.md b/.claude/skills/write-e2e-test/SKILL.md new file mode 100644 index 00000000..0f7fc7dd --- /dev/null +++ b/.claude/skills/write-e2e-test/SKILL.md @@ -0,0 +1,70 @@ +--- +name: write-e2e-test +description: Write a Playwright E2E test for Bruno API Docs in the class-based page-object style. + Use when adding or modifying tests under packages/bruno-api-docs/e2e/, creating a new test + suite, or reproducing a bug as an e2e spec. +--- + +# Writing an E2E test + +Read `.claude/rules/testing.md` first for the quick reference, and +`packages/bruno-api-docs/e2e/README.md` for the full narrative. Tests use a **class-based +page-object model**: page objects and components are classes handed to specs via fixtures. Specs +never call `new` and never inline a raw selector. Run everything from `packages/bruno-api-docs/`. + +## The model + +- **Page object** (`e2e/pages/.page.ts`): a class extending `BasePage`, describing one + screen. It sets its own `root`, composes the components a test needs as readonly fields, and + owns navigation: `goto(path)` for screens with a URL, `open(path: string[])` for screens reached + through the sidebar. +- **Component** (`e2e/components/.component.ts`): a class extending `BaseComponent` + (every component has a `root: Locator`). Common controls live at the top level; sections that + belong to a single page go in a subfolder named after the page (`components/overview/`, + `components/playground/`, ...). Derive child locators from `root` or a `testId` base; see + `components/secret-value.component.ts`. +- **Fixture** (`e2e/playwright/pages.fixture.ts`): instantiates each page object and component + and exposes it to specs. `e2e/playwright/index.ts` merges fixtures with `mergeTests` and + re-exports `test`/`expect`. + +## Steps + +1. **Pick the fixture collection.** The dev entry mounts `src/e2eFixtures/*` via + `/?fixture=folders|vars|descriptions|qa`; no query mounts the sample collection. Reuse one + that already has the shape you need. Add a new fixture only when none does, named after the + shape (`descriptionsCollection`), never after a ticket. +2. **Place the spec** at `e2e/tests//.spec.ts`, reusing an existing area folder + (`overview/`, `request/`, `playground/`, `sidebar/`, `search/`, `environments/`, ...) or + adding one. +3. **Add or reuse a page object.** If the screen has one in `e2e/pages/`, use it. Otherwise + create `.page.ts` extending `BasePage`, set `root`, compose its components, add + navigation. +4. **Add or reuse components.** Extract each meaningful UI section into a component extending + `BaseComponent` rather than putting locators on the page object; page-specific sections go in + the page's subfolder under `components/`. +5. **Register the fixture** in `e2e/playwright/pages.fixture.ts` so specs receive it off the + test callback. +6. **Locate by `data-testid`.** `page.getByTestId('...')`; derive child ids from a base. If the + app has no stable id for what you need, add a `testId` prop to the component (see + `.claude/rules/app-conventions.md`) rather than locating by class or text. Rendered-Markdown + internals are matched by role scoped within their test-id'd container. +7. **Write the spec.** `import { test, expect } from '../../playwright';` (relative; no aliases in + `e2e/`). Pull objects off the callback: `test('…', async ({ requestPage }) => { … })`. Navigate + via `goto`/`open` in `beforeEach`. Keep every `expect` in the spec. Title it in plain English: + what the page does. +8. **Use auto-retrying assertions** (`await expect(locator).toBeVisible()`, `toHaveText`, + `toHaveAttribute`); never assert immediately after navigation. Reserve `page.waitForTimeout()` + for when no locator assertion can wait. +9. **Run it**: `npx playwright test e2e/tests//.spec.ts` (the dev server starts via + `webServer`). Debug with `npm run test:e2e:ui`, `--headed`, or `--debug`. + +## Checklist before done + +- [ ] Spec imports `test`/`expect` from `../../playwright` and gets objects off the callback +- [ ] No `new` in the spec; no raw selectors; everything via a page object or component field +- [ ] Elements located by `data-testid` (added to the component if missing), not styling classes +- [ ] New page object extends `BasePage`; new component extends `BaseComponent`; fixture registered +- [ ] Page-specific sections live in the page's subfolder under `components/` +- [ ] Reused components get a unique `testId` base per instance +- [ ] Folders expanded via the sidebar before asserting on their children +- [ ] Auto-retrying assertions; no `test.only`, no `page.pause()`, no ticket ids in names diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 00000000..1cb7f91d --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,109 @@ +# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json + +language: 'en-US' +early_access: false +tone_instructions: 'You are an expert code reviewer in TypeScript, React, Redux Toolkit, Emotion, Vite, Vitest, and Playwright. You work in an enterprise software developer team, providing concise and clear code review advice. You only elaborate or provide detailed explanations when requested.' + +knowledge_base: + opt_out: false + code_guidelines: + enabled: true + filePatterns: + - '**/CODING_STANDARDS.md' + +reviews: + profile: 'chill' + request_changes_workflow: false + high_level_summary: true + poem: true + review_status: true + collapse_walkthrough: false + auto_review: + enabled: true + drafts: false + base_branches: ['main', 'release/*'] + path_filters: + - '!**/dist/**' + - '!**/dist-standalone/**' + - '!**/playwright-report/**' + - '!**/test-results/**' + - '!**/package-lock.json' + - '!packages/bruno-api-docs/src/styles/theme.generated.css' + path_instructions: + - path: '**/*' + instructions: | + Bruno API Docs is the documentation renderer and embedded playground that Bruno + generates from an API collection. It runs in the reader's browser on macOS, Windows, + and Linux, ships as a standalone bundle loaded from a CDN, and is rendered to static + markup in unit tests. Reason about every change as "what should an API client do + here?" (requests, responses, auth, params, headers, bodies, environments), not as a + generic document tool or an API server. + + `CODING_STANDARDS.md` is the source of truth for how code is written; apply it. Do not + restate ESLint-enforced style; the pre-commit hook fixes that. Focus on what a linter + cannot see: correctness, root cause over symptom patch, secret leakage, XSS through + rendered collection content, format-shape assumptions, missing tests, dead code left + behind, comments that narrate the change, and ticket identifiers in code. + - path: 'packages/bruno-api-docs/src/**/*.{ts,tsx}' + instructions: | + Review against CODING_STANDARDS.md §TypeScript, §Imports and aliases, §React components, + §Styling and theming, §State, §Reading the OpenCollection format, and §Cross-OS. Flag + deviations rather than proposing new patterns. In particular: + - Any colour or font not expressed as a CSS custom property; any static inline `style`. + - Any `.description` read that assumes a bare string or a `{ content }` object without + going through the normalisers in `utils/description.ts`, `utils/request.ts`, or + `utils/schemaHelpers.ts`. + - A change to how the docs pages read a field that is not mirrored in the playground + slice, or vice versa (`store/slices/docs.ts` vs `store/slices/playground.ts`). + - `x || default` where an empty auth token, param, or header is a legitimate state. + - `window`, `document`, `navigator`, or storage read at module scope or during render. + - A removed render path that leaves orphaned components, props, or store state behind. + - An edit to `src/styles/theme.generated.css` (tokens live in `src/theme/tokens/`). + - path: 'packages/bruno-api-docs/src/{runner,scripting}/**/*.ts' + instructions: | + This code builds real HTTP requests from collection data and runs reader-editable + scripts in a QuickJS sandbox. Treat every interpolated variable, header value, URL, and + script output as untrusted. Flag header or URL injection through `\r\n` or scheme + changes, any new host capability exposed to scripts, secrets reaching logs or error + messages, and behaviour that diverges from what the Bruno desktop app does for the same + request. + - path: 'packages/bruno-api-docs/src/**/*.{spec,test}.{ts,tsx}' + instructions: | + Unit tests run in Vitest with `environment: 'node'`; see CODING_STANDARDS.md §Tests and + `.claude/rules/unit-testing.md`. Ensure that: + - New component specs render through `useRenderToDom` and query with + `src/test-utils/dom.ts`, not raw `renderToStaticMarkup` string matching. + - Assertions are unconditional. Flag `?.`, `??`, `||`, `if`, or try/catch that could let + an assertion be skipped or an error be swallowed. + - Every behaviour the PR adds or changes has a spec exercising it; a bug fix ships with + a regression test. Interactive behaviour belongs in Playwright, not here. + - Assertions target the unique value the change produces, not a substring the fixture + already carries elsewhere. + - path: 'packages/bruno-api-docs/e2e/**/*.ts' + instructions: | + Review the following e2e test code written using the Playwright test library. Ensure that: + - Follow the guidance in `packages/bruno-api-docs/e2e/README.md` - the canonical E2E + guide (class-based page-object model: `pages/` extend `BasePage`, `components/` + extend `BaseComponent`, fixtures in `playwright/`, specs in `tests/` by feature). + - For anything the guide above doesn't cover, follow standard Playwright and e2e + automation best practices. + - Elements are located by `data-testid` through page-object and component classes; + never by styling classes, tag names, or index. Rendered Markdown internals are matched + by role within their test-id'd container. + - Specs import `test` and `expect` from the `playwright` folder and take page objects + and components off the test callback; no `new` and no raw selectors in specs. + - Every `expect` lives in the spec; page objects expose elements and actions only. + - All imports are relative; `e2e/` has no path aliases. + - Test titles read like documentation: plain English describing what the page does. + - Try to reduce usage of `page.waitForTimeout()` in code unless absolutely necessary + and the locator cannot be found using existing `expect()` playwright calls. + - Avoid using `page.pause()` in code. + - Avoid using `test.only`. + - Use locator variables for locators. + - Use multiple assertions. + - Promote the use of `test.step` as much as possible so the generated reports are + easier to read. + - Every user-visible behaviour the PR adds or changes has an e2e spec covering it. + +chat: + auto_reply: true diff --git a/.gitignore b/.gitignore index 3a885038..48b3c580 100644 --- a/.gitignore +++ b/.gitignore @@ -4,6 +4,6 @@ dist dist-standalone test.js -# Claude Code -.claude/ -CLAUDE.md \ No newline at end of file +# Claude Code (per-machine files; the shared config in .claude/ is committed) +.claude/settings.local.json +CLAUDE.local.md diff --git a/CODING_STANDARDS.md b/CODING_STANDARDS.md new file mode 100644 index 00000000..40292af2 --- /dev/null +++ b/CODING_STANDARDS.md @@ -0,0 +1,171 @@ +# Bruno API Docs Coding Standards + +How code in this repo is written. Humans read it, CodeRabbit applies it on every PR, and the +Claude config in `.claude/` points at it. Change a rule here; everything else references this file. + +Code written before a rule may stay as it is until touched. When you touch it, bring the lines +you change up to standard and leave the rest of the file alone. + +## General Style Rules + +- No diffs unless an actual change is made. Keep changes minimal and avoid reformatting or + whitespace churn in lines the change does not need. +- 2 spaces for indentation. Single quotes for TypeScript strings, double quotes for JSX attributes. +- Semicolons at the end of every statement. No trailing commas. +- Parentheses around arrow-function parameters, even a single one. Opening braces on the same line. +- Lines stay under 120 characters. URLs and string literals are exempt. +- `import type` for type-only imports. `console.warn` and `console.error` are fine; `console.log` + is not. + +ESLint enforces all of the above and the pre-commit hook auto-fixes it. Do not hand-police style +in review; spend the attention on the sections below. + +## Comments + +- Write for a cold reader. A comment explains what the code cannot: rationale, an invariant, a + unit, a workaround and the constraint forcing it, a pointer to a spec. +- Never narrate the change. No `// added to fix X`, `// as requested`, `// updated to handle Y`. + If the reason matters, state it as a fact about the code or link the issue. +- Never restate the code. If it is self-explanatory, leave it bare. +- No commented-out code, no `TODO`-for-me notes, no `// ... existing code ...` scaffolding. +- `StyledWrapper.ts` files contain no comments at all. +- No Jira keys or ticket identifiers anywhere in source, comments, or test names. + +## TypeScript + +- Type props with an explicit `interface FooProps`; type components as `React.FC`. +- Avoid `any`. Existing occurrences are legacy; do not add more. The real shapes live in + `@opencollection/types/...`. +- Optional chaining (`?.`) only where the value can genuinely be missing **and** the missing + case is handled right there. On a value the types say is present it hides bugs. +- `x || default` turns an empty string, `0`, or `false` into the default. For an API client an + empty auth token or a blank header is a real state. Use `??` or an explicit `undefined` check + when "not set" must survive. + +## Imports + +- Reach into `src/` through the `@/*` alias (`@/utils/cx`, `@/store/hooks`). Sibling files stay + relative (`./StyledWrapper`). No `../../` chains where the alias covers the target. +- Slices import through `@/store/slices/`. `@slices/*` is rejected by lint. +- Runtime code imports only from `dependencies`. The package is published; a `devDependencies` + import breaks consumers. +- `e2e/` has no aliases. Everything there is relative. + +## React + +- One component per folder: `Foo/Foo.tsx`, `Foo/StyledWrapper.ts`, `Foo/Foo.spec.tsx`. Add + `index.ts` only for pages and public entry points. +- Export both named and default (`export const Foo` + `export default Foo`). +- Import hooks by name (`import { useState } from 'react'`), never `React.useState`. +- MUST: search `src/ui/`, `src/components/`, and `src/hooks/` for an existing piece before + adding one. Compose `Section`, `Heading`, `EmptyState`, `Tabs`, `CopyButton`, and friends. + Where an existing primitive is almost right, widen it; do not stand up a near-duplicate. +- MUST: a component is either controlled or uncontrolled, never both. +- SHOULD: derive values from props and handle events in handlers before reaching for + `useEffect`. An effect that only mirrors a prop into state is a smell. +- Every element an e2e test might target takes `testId?: string` and renders it as + `data-testid` on the root. Child ids derive from it and disappear when it is unset: + + ```tsx + data-testid={testId ? `${testId}-value` : undefined} + ``` + + Test ids are unique on the page, so a component reused across sections gets a distinct + `testId` per instance. +- Interactive elements carry `aria-label`; toggles carry `aria-pressed`; decorative icons carry + `aria-hidden="true"`; buttons declare `type="button"`. + +## Styling and Theming + +- Styles live in the component's Emotion `StyledWrapper.ts`. The wrapper styles child classNames + (`.foo-row`, `.foo-value`); do not create a styled component per node. +- Colours and fonts come from theme CSS variables only (`var(--text-primary)`, + `var(--border-color)`, `var(--font-sans)`). Tokens live in `src/theme/tokens/`; + `src/styles/theme.generated.css` is generated by `npm run gen:theme` and never edited by hand. +- Tailwind utilities are for layout. A colour, font, or border never comes from a utility class. +- No static inline `style={{ ... }}`. Put the rule on a className. Inline style is only for + values computed at runtime that no class can express (measured sizes, user-picked colours). + +## State + +- Redux Toolkit. Read with `useAppSelector`, dispatch with `useAppDispatch` from + `src/store/hooks`; never the untyped `react-redux` hooks. +- Persistence (storage, `data-theme`) happens in `store.ts` subscribers, not in reducers. + +## Reading the OpenCollection Format + +The app reads collections and never writes them. The format has evolved, so fields are unions. + +- `description` is a bare string or a legacy `{ content, type }` object. Never read + `.description` directly; go through `descriptionText` / `resolveDescription` + (`utils/description.ts`), `getDescription` (`utils/request.ts`), or `getItemDescription` + (`utils/schemaHelpers.ts`). A new description-bearing field gets the same treatment. +- Docs pages and the playground read the same collection through separate slices. A change to + how one reads a field is mirrored in the other. +- Parse defensively with safe defaults so an older or newer collection renders instead of + throwing. Ask what an API client should do with this request, header, auth block, or + environment, not what a document viewer would do. + +## Cross-OS and Environment Safety + +The reader's browser runs on macOS, Windows, or Linux; unit tests render to static markup with +no `window`. + +- Split user-provided multiline text on `/\r?\n/`, never `'\n'`. Emit `\n`. +- Shortcuts accept `event.metaKey || event.ctrlKey`. Shortcut labels come from `isMacPlatform()` + in `utils/platform.ts`, not a hardcoded `⌘` or `Ctrl`. +- Never touch `window`, `document`, `navigator`, or storage at module scope or during render. + Guard it or move it into an effect. + +## Readability and Abstractions + +- Names say what they hold: the concrete subject and type, readable on first sight. +- Extract when it improves readability or serves a clear, anticipated reuse. Do not add a layer + that only adds indirection: a one-line forwarder, an option nobody passes, a branch for a + state nothing produces. +- Anything added has a live consumer in the same change. Nothing "for later". +- Keep the pipeline obvious and linear. Functional style is welcome; ADTs and monads are not. +- Dead code leaves with the feature that used it: orphaned components, props, store wiring, + styles, and tests go in the same change. + +## Tests + +- Every behaviour a change adds or alters has a test, mapped while you work: each new branch, + default, UI state, and bug fix names the spec that exercises it. A green suite is not the + check; the mapping is. +- Write behaviour-driven tests. Assert observable output, not implementation detail. Cover the + happy path and the realistic failure paths a collection can produce (missing description, + disabled header, empty auth). +- Assertions are unconditional. No `?.`, `??`, `||`, `if`, or try/catch that could let an + assertion be skipped or an error swallowed. +- Assert the unique value the change produces, not a substring the fixture already carries. + +### Unit Tests (Vitest) + +- Specs sit next to the code as `*.spec.ts(x)`. Vitest runs in `environment: 'node'`. +- Render components with `useRenderToDom` (`src/hooks/useRenderToDom.ts`) and query with + `src/test-utils/dom.ts`. Do not assert on raw `renderToStaticMarkup` strings in new specs. +- Clicks, typing, focus, and scroll are Playwright territory, not unit specs. + +### E2E Tests (Playwright) + +`packages/bruno-api-docs/e2e/README.md` is the canonical guide. The rules that matter most: + +- Class-based page-object model: page objects extend `BasePage`, components extend + `BaseComponent`, fixtures hand them to specs. Specs never call `new` and never inline a + selector. +- Locate by `data-testid`, never by styling class, tag, or index. +- Every `expect` lives in the spec. Page objects expose elements and actions only. +- Auto-retrying assertions instead of `page.waitForTimeout()`. No `test.only`, no `page.pause()`. +- Titles read like documentation: what the page does, in plain English. +- Fixture collections live in `src/e2eFixtures/` and mount through `?fixture=`. Ad-hoc user + collections are exercised through the standalone build, never added to `dev.tsx`. + +## Pull Requests + +- Branch from `main` (or the active `release/*`); PRs target `main`. +- Before pushing: `npm run lint` at the root, `npm run test:run` in `packages/bruno-api-docs`, + and `npm run test:e2e` when UI behaviour changed. +- Every PR that changes published behaviour carries a changeset (`npm run changeset` or a + `changeset:patch|minor|major` label). Tooling-only PRs use an empty changeset. +- The description covers the problem and the behavioural change. No checklists or test logs. diff --git a/contributing.md b/contributing.md index be5b1161..a4e428bd 100644 --- a/contributing.md +++ b/contributing.md @@ -31,7 +31,7 @@ Run these from `packages/bruno-api-docs`: 1. Open or comment on an issue first for anything beyond a small fix. 2. Create a branch from `main`. -3. Make your change and add or update tests. Run `npm run lint` and `npm run test:run` before pushing; run `npm run test:e2e` when UI behavior changed. +3. Make your change following [`CODING_STANDARDS.md`](./CODING_STANDARDS.md) and add or update tests. Run `npm run lint` and `npm run test:run` before pushing; run `npm run test:e2e` when UI behavior changed. 4. Open a pull request against `main` describing the change and linking the issue. Continuous integration runs the end-to-end tests on pull requests via GitHub Actions.