diff --git a/.claude/skills/add-feature/SKILL.md b/.claude/skills/add-feature/SKILL.md index 2cee90ca61..0d5af264f5 100644 --- a/.claude/skills/add-feature/SKILL.md +++ b/.claude/skills/add-feature/SKILL.md @@ -6,11 +6,25 @@ argument-hint: [feature-name] # Add Feature -Order of operations only; CLAUDE.md covers package boundaries, `.claude/rules/web-app.md` mutation architecture and +Order of operations only, spec first; CLAUDE.md covers package boundaries, `.claude/rules/web-app.md` mutation architecture and `.claude/rules/icons.md` icon rules. Paths below are relative to `apps/web/src`, except repo-rooted ones, which begin with `apps/`, `packages/`, `scripts/` or `.github/`. -## 1. Feature directory +## 1. Failing spec first + +Start from the issue's **Acceptance tests** list and write one `it()` per behaviour before any feature code. Run it and confirm it fails **for the right reason**: an assertion about the missing behaviour, not an import error, a missing mock or a typo. When the subject file does not exist yet, add a stub export (a component returning `null`, a function returning `undefined`) so the run reaches the assertion. Steps 2 to 7 then make it green. If the issue says `No test-first: ` (pure layout or CSS, a dependency upgrade, infra config), skip this step and repeat that line in the PR. + +Vitest's `include`, set in `apps/web/vitest.config.mts` and relative to `apps/web/`, is +`src/specs/**/*.spec.{ts,tsx}`, so a spec beside the component is not collected. Put it in +`specs/features//`, render with `renderWithQueryClient` from +`@/specs/test-utils` and seed with `seedQueryClient(queryClient, data)`, client first. +i18next is globally mocked to return the key, so assert on `".button-label"`. +The same global setup (`specs/setup-any-spec.ts`) mocks `@/utils` down to `random` and +`getAccessToken`, so a spec that reaches a feature-local api wrapper pulling anything else +from `@/utils` has to re-mock it with `vi.importActual("@/utils")`. `pnpm test` at the root +is web-only. + +## 2. Feature directory Files sit flat at the feature root with an `index.ts` barrel. Most feature dirs have that barrel; a few carry `components/`, `hooks/`, `api/` or `types/` subfolders. Do not scaffold @@ -31,7 +45,7 @@ here. A feature stylesheet Tailwind cannot express is registered as an `@import` `styles/_shared-components.scss`, which `styles/style.scss` pulls in once. Do not import it from the component. -## 2. Flag (only if toggleable) +## 3. Flag (only if toggleable) Add to `visionFeatures` in `config/config.ts`, then mirror into `config/config.template.ts` by hand: nothing imports the template, so a missed edit is not caught. Gate with @@ -45,7 +59,7 @@ by hand: nothing imports the template, so a missed edit is not caught. Gate with Outside JSX: `getConfigValue(({ visionFeatures }) => ...)`. -## 3. Data +## 4. Data - Blockchain: run `/add-sdk-mutation` or `/add-query` first. - Private API: the client is `packages/sdk/src/modules/private-api`. There is no @@ -57,7 +71,7 @@ Outside JSX: `getConfigValue(({ visionFeatures }) => ...)`. - `useActiveAccount()` from `@/core/hooks/use-active-account`. Keys: `QueryKeys` from `@ecency/sdk`, or `QueryIdentifiers` from `@/core/react-query` for web-only queries. -## 4. i18n +## 5. i18n The app imports the i18next default export directly. `react-i18next` and `useTranslation` are not used here. @@ -75,25 +89,13 @@ one object is silently last-wins, guarded by `specs/features/i18n-locale-duplicate-keys.spec.ts`. Inline markup uses `Tsx` from `@/features/i18n/helper`. -## 5. Page route (if it owns a URL) +## 6. Page route (if it owns a URL) Plain `apps/web/src/app//page.tsx`. `(dynamicPages)` holds profile, entry, feed and community; other routes sit at the app root or under `(staticPages)`. Page-local components go in `_components/` beside `page.tsx`. `routes.ts` is a legacy map read by a few files; add to it for pattern matching. -## 6. Specs - -Vitest's `include`, set in `apps/web/vitest.config.mts` and relative to `apps/web/`, is -`src/specs/**/*.spec.{ts,tsx}`, so a spec beside the component is not collected. Put it in -`specs/features//`, render with `renderWithQueryClient` from -`@/specs/test-utils` and seed with `seedQueryClient(queryClient, data)`, client first. -i18next is globally mocked to return the key, so assert on `".button-label"`. -The same global setup (`specs/setup-any-spec.ts`) mocks `@/utils` down to `random` and -`getAccessToken`, so a spec that reaches a feature-local api wrapper pulling anything else -from `@/utils` has to re-mock it with `vi.importActual("@/utils")`. `pnpm test` at the root -is web-only. - ## 7. Wiring in - Toolbar button: `features/shared/editor-toolbar/index.tsx`, inside a `Conditional`. The @@ -101,7 +103,7 @@ is web-only. - Profile section: `app/(dynamicPages)/profile/[username]/_components/`. - Navigation: `features/shared/navbar/navbar-main-sidebar.tsx`. -Finish with `pnpm lint`, `pnpm typecheck` and `pnpm test`. Those three run none of the script +Before the PR, the spec from step 1 must be green and the PR body lists each spec path, the command and the assertion that failed before the change, then the green run after it. When step 1 was skipped, the PR repeats the issue's `No test-first: ` instead. Finish with `pnpm lint`, `pnpm typecheck` and `pnpm test`. Those three run none of the script audits in `.github/workflows/typecheck.yml`; the ones a feature is likely to trip are `node scripts/icon-scss-audit.mjs`, `node scripts/icon-tsx-audit.mjs --fail` and `node scripts/slim-entries-audit.mjs --fail`. Copy each line's flags exactly. The two diff --git a/.claude/skills/add-query/SKILL.md b/.claude/skills/add-query/SKILL.md index 237aab9645..993af88252 100644 --- a/.claude/skills/add-query/SKILL.md +++ b/.claude/skills/add-query/SKILL.md @@ -23,7 +23,28 @@ an SDK file plus one line in the wallets re-export. CLAUDE.md holds the package rules: the multi-chain code stays in `@ecency/wallets`. Its external chain list is stale, since the chains under `assets/external/` are BTC, ETH, BNB and SOL. -## 1. Add the key +## 1. Failing spec first + +Write the builder's spec before the builder: the `queryKey` it must produce, what `queryFn` returns for a mocked response (empty and error shapes too) plus the `enabled` guard for missing arguments. Run it and confirm it fails **for the right reason**: an assertion about the missing behaviour, not an import error, a missing mock or a typo. When the subject file does not exist yet, add a stub export (a component returning `null`, a function returning `undefined`) so the run reaches the assertion. If the issue says `No test-first: ` (pure layout or CSS, a dependency upgrade, infra config), skip this step and repeat that line in the PR. + +In either package, colocate `get--query-options.spec.ts` beside the builder (the SDK +picks up vitest's default spec glob, `packages/wallets/vitest.config.ts` includes +`src/**/*.spec.ts`), then run the one spec (plus `pnpm typecheck` once it is green): + +```bash +pnpm --filter @ecency/sdk test src/modules//queries/get--query-options.spec.ts +pnpm --filter @ecency/wallets test src/modules/wallets/queries/.spec.ts +pnpm --filter @ecency/wallets test src/modules/assets/external//.spec.ts # BTC, ETH, BNB, SOL +``` + +**Never add `--` before the path.** pnpm forwards it to vitest as a passthrough separator +rather than a file filter, so the whole package suite runs instead of the one spec. + +A web query does not colocate. `apps/web/vitest.config.mts` includes +`src/specs/**/*.spec.{ts,tsx}` only, so the spec goes under `apps/web/src/specs/api/queries/` +and runs from the workspace root as `pnpm test src/specs/api/queries/.spec.ts`. + +## 2. Add the key Key style is per workspace, so pick the right one before writing anything: @@ -31,7 +52,7 @@ Key style is per workspace, so pick the right one before writing anything: |---|---| | `@ecency/sdk` | the shared `QueryKeys` object, below | | `@ecency/wallets` | a package-local namespaced array; `QueryKeys` is not imported there | -| `apps/web` | the `QueryIdentifiers` enum, or a local suffix constant appended to another builder's key (step 3) | +| `apps/web` | the `QueryIdentifiers` enum, or a local suffix constant appended to another builder's key (step 4, where the enum member is added) | `@ecency/wallets` keeps its keys local so the multi-chain code does not reach back into the SDK, so CLAUDE.md's blanket rule to take every cache key from `QueryKeys` is @@ -71,7 +92,7 @@ prefix-safe. Some keys stay prefix-safe without the helper by branching on the m argument and returning the shorter array: `search.similarEntries`, `wallet.aggregatedHistory` plus `polls.vote`. -## 2. Write the builder +## 3. Write the builder File: `packages/sdk/src/modules//queries/get--query-options.ts`. In `@ecency/wallets` there are two homes: a wallet-level builder is @@ -79,7 +100,7 @@ File: `packages/sdk/src/modules//queries/get--query-options.ts`. ETH, BNB or SOL sits directly in `packages/wallets/src/modules/assets/external//`, with no `queries/` level. A web-only query is `apps/web/src/api/queries/-query.ts` (`get-contributors-query.ts`, -`get-gifs-query.ts`), usually with its key from `QueryIdentifiers` (step 3). The builder +`get-gifs-query.ts`), usually with its key from `QueryIdentifiers` (step 4, where the enum member is added). The builder shape below carries across those homes; the imports do not. Export a function, never a hook, so one options object serves a component, a server prefetch or a non-React caller. Almost all top-level `export function` names under @@ -178,7 +199,7 @@ type PageParam = { }, ``` -## 3. Export it +## 4. Export it Add one line to the domain's `queries/index.ts`, which `modules//index.ts` already re-exports. A brand new domain also needs `export * from "./modules/";` in @@ -201,7 +222,7 @@ directory can import the BTC, ETH, BNB or SOL builders. Nothing does. Adding a b there means adding the missing `export * from "./external";` line as well, otherwise the new export is unreachable. -## 4. Consume it +## 5. Consume it ```typescript const { data } = useQuery(getPostQueryOptions(author, permlink)); @@ -214,24 +235,6 @@ const entry = await prefetchQuery(getPostQueryOptions(author, permlink)); Those helpers (`apps/web/src/core/react-query/query-helpers.ts`) wrap the fetch in an SSR timeout, so a hanging node cannot hold a render open. -## 5. Test - -In either package, colocate `get--query-options.spec.ts` beside the builder (the SDK -picks up vitest's default spec glob, `packages/wallets/vitest.config.ts` includes -`src/**/*.spec.ts`), then run the one spec plus `pnpm typecheck`: - -```bash -pnpm --filter @ecency/sdk test src/modules//queries/get--query-options.spec.ts -pnpm --filter @ecency/wallets test src/modules/wallets/queries/.spec.ts -``` - -**Never add `--` before the path.** pnpm forwards it to vitest as a passthrough separator -rather than a file filter, so the whole package suite runs instead of the one spec. - -A web query does not colocate. `apps/web/vitest.config.mts` includes -`src/specs/**/*.spec.{ts,tsx}` only, so the spec goes under `apps/web/src/specs/api/queries/` -and runs from the workspace root as `pnpm test src/specs/api/queries/.spec.ts`. - ## Gotchas - `enabled` gates automatic fetching only. The helpers in diff --git a/.claude/skills/add-sdk-mutation/SKILL.md b/.claude/skills/add-sdk-mutation/SKILL.md index 5c28534899..4505ce62a1 100644 --- a/.claude/skills/add-sdk-mutation/SKILL.md +++ b/.claude/skills/add-sdk-mutation/SKILL.md @@ -8,6 +8,10 @@ argument-hint: [operation-name] Layering is in `.claude/rules/web-app.md` under "Mutation Architecture". Copy the real files below. +## 0. Failing spec first + +Before the hook, write `use-.spec.ts` beside where it will live, for the logic the hook adds: the operation payload it builds, any cache update and the keys it invalidates. Copy the layout of `posts/mutations/use-vote.spec.ts`, which tests the vote's pure helpers rather than the React hook. Run it and confirm it fails **for the right reason**: an assertion about the missing behaviour, not an import error, a missing mock or a typo. When the subject file does not exist yet, add a stub export (a component returning `null`, a function returning `undefined`) so the run reaches the assertion. A hook that only forwards to `useBroadcastMutation` with no logic of its own needs no spec: that is the exemption below, with the reason `forwarding-only hook`. If the issue says `No test-first: ` (pure layout or CSS, a dependency upgrade, infra config), skip this step and repeat that line in the PR. + ## 1. SDK mutation hook `packages/sdk/src/modules//mutations/use-.ts`. Operations come @@ -68,9 +72,8 @@ Imports as the SDK uses them: `useBroadcastMutation` from `@/modules/core` or `AuthContextV2` as `import type`, usually from `@/modules/core/types`. `invalidateAfterBroadcast` already defers on async broadcasts, so no hand rolled `setTimeout`. Other models: -`posts/mutations/use-vote.ts`, `use-reblog.ts`, `use-comment.ts`. Co-locate a -`use-.spec.ts` when the hook has logic of its own; most hooks have none, -so copy the pattern from `posts/mutations/use-vote.spec.ts`. +`posts/mutations/use-vote.ts`, `use-reblog.ts`, `use-comment.ts`. The spec from step 0, when there is +one, goes green here. Export with `export * from "./use-";` in the domain's `mutations/index.ts`. The chain up to `packages/sdk/src/index.ts` is usually @@ -116,7 +119,7 @@ named export to `apps/web/src/api/sdk-mutations/index.ts`. ## 4. Verify -The web app resolves `@ecency/sdk` through `dist`, +The step 0 spec passes and the PR body lists each spec path, the command and the assertion that failed before the change, then the green run after it; with no spec, the PR carries the `No test-first: ` line. The web app resolves `@ecency/sdk` through `dist`, so `pnpm typecheck` reports your new export as missing from the package until the SDK is rebuilt: diff --git a/.claude/skills/add-test/SKILL.md b/.claude/skills/add-test/SKILL.md index 494fe99171..59a092b560 100644 --- a/.claude/skills/add-test/SKILL.md +++ b/.claude/skills/add-test/SKILL.md @@ -15,6 +15,8 @@ Procedure for `apps/web` (`@ecency/web`) specs. Background you should not re-rea `AccountBuilder`). It does not mention `mockFeatureFlags` or `waitForMs`, so read `test-utils.tsx` itself before assuming a helper is missing. +When a spec covers behaviour being added or fixed, write it before the code, from the issue's **Acceptance tests** list. Run it and confirm it fails **for the right reason**: an assertion about the missing behaviour, not an import error, a missing mock or a typo. When the subject file does not exist yet, add a stub export (a component returning `null`, a function returning `undefined`) so the run reaches the assertion. A spec that pins behaviour which already works (a test-only PR, a refactor) has no red run; say so in the PR. + Sections 1-4 cover what usually breaks. ## 1. Where the file goes @@ -175,4 +177,5 @@ vi.mocked(useQuery).mockImplementation(() => { - [ ] `import "@testing-library/jest-dom"` when asserting on the DOM - [ ] `@/utils` and `@ecency/sdk` re-mocked with `importActual` for anything past the global stubs - [ ] Assertions target i18n keys, not English strings +- [ ] For new or fixed behaviour: spec was run red before the change, failing on its assertion - [ ] `pnpm test ` passes with no `--`, then `pnpm test` for the full suite diff --git a/.claude/skills/code-review/SKILL.md b/.claude/skills/code-review/SKILL.md index 1727b63b0a..e325689a47 100644 --- a/.claude/skills/code-review/SKILL.md +++ b/.claude/skills/code-review/SKILL.md @@ -85,6 +85,19 @@ modals use CSS transitions (`apps/web/src/features/ui/modal/index.tsx`). Do not reintroduce it. **Tests** +- **A spec for added or fixed behaviour must fail without the change.** Check it: revert + only the source part of the diff (keep the spec), run the spec and confirm it goes red on + the assertion it names, then restore. A source file the PR adds cannot be reverted that + way, since the spec would then fail on its import: replace it with a stub export (a + component returning `null`, a function returning `undefined`) instead. For a new guard, + delete just the guard. A spec that stays green only exercises the path where the change + does nothing: flag it as `BUG`. Restore from a copy of the file, never + `git checkout -- `, which drops uncommitted work. +- Specs that pin behaviour which already works (a test-only PR, a refactor) are expected + to stay green and are not a finding. +- The PR body lists each spec path, the command, the assertion that failed before the + change and the green run after it. A change with no spec repeats the issue's + `No test-first: `. - Placement decides collection. Some workspaces pin `include`, so a misplaced or wrongly suffixed file there is silently never run: `apps/web` takes `src/specs/**/*.spec.{ts,tsx}` only, `packages/wallets` `src/**/*.spec.ts`, diff --git a/.claude/skills/debug/SKILL.md b/.claude/skills/debug/SKILL.md index 7c174795c5..511e048110 100644 --- a/.claude/skills/debug/SKILL.md +++ b/.claude/skills/debug/SKILL.md @@ -23,6 +23,10 @@ invisible. Most invariants below have a spec; find it before a repro; web specs one browser only, or only right after a deploy, is often already handled by the scripts in `apps/web/public/scripts/`; check those first. +## First: a failing spec that reproduces the bug + +Before changing any code, write a spec that fails the way the report does, driven through the real code path with the reporter's real input (not a simplified harness that can drop the cause). Use the triage below to find where. Run it and confirm it fails **for the right reason**: an assertion about the missing behaviour, not an import error, a missing mock or a typo. When the subject file does not exist yet, add a stub export (a component returning `null`, a function returning `undefined`) so the run reaches the assertion. If it passes on the unchanged code, that is not a fix: either the report does not reproduce on current code or the spec misses the reporter's input. Stop, recheck the input, steps and assertion, then report that before writing any fix. The fix is done when that spec goes from red to green and the rest of the suite still passes; keep the spec. A bug that only shows in a real browser or on a live node (layout, CDN, network) may not fit a spec; then record the manual repro steps in the issue and say why there is no spec. + ## Post shown as deleted, 404, or spinning `getPostQueryOptions(author, permlink?, observer = "", num?)` diff --git a/.github/ISSUE_TEMPLATE/spec.yml b/.github/ISSUE_TEMPLATE/spec.yml new file mode 100644 index 0000000000..d13ed94a8f --- /dev/null +++ b/.github/ISSUE_TEMPLATE/spec.yml @@ -0,0 +1,33 @@ +name: Spec (feature or bug) +description: Work item with acceptance tests written before the code, or a stated reason why there are none +body: + - type: textarea + id: problem + attributes: + label: Problem + description: What is wrong or missing and who sees it. For a bug, give the exact steps, the input that triggers it, what happens and what should happen. + validations: + required: true + - type: textarea + id: proposal + attributes: + label: Proposal + description: The change, at the level of behaviour. Name files only when the location is already known. + validations: + required: true + - type: textarea + id: acceptance-tests + attributes: + label: Acceptance tests + description: | + Each line becomes a spec that is written BEFORE the code, run and seen failing on its assertion, then made green by the change. For a bug, the first line reproduces the report with the real input. + If a spec written first adds nothing (pure layout or CSS, a dependency upgrade, infra config), write one line instead: `No test-first: ` + placeholder: | + - [ ] Given , when , then . Spec: `` + validations: + required: true + - type: textarea + id: out-of-scope + attributes: + label: Out of scope + description: Optional. Related things this issue deliberately leaves alone. diff --git a/CLAUDE.md b/CLAUDE.md index 1c0f73f255..cdb1656dff 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -104,7 +104,7 @@ node scripts/origin-config-audit.mjs --fail pnpm test src/specs/path/to/test.spec.tsx # For packages (Vitest) -pnpm --filter @ecency/sdk test -- path/to/test.spec.ts +pnpm --filter @ecency/sdk test path/to/test.spec.ts # no `--`: with it, the whole package suite runs ``` ## Monorepo Architecture @@ -241,7 +241,9 @@ The web app will not see package changes until they are rebuilt. ## Pull Requests - Branch from `develop` (not `master`) -- Include tests with changes +- Test first: issues carry a required **Acceptance tests** field (`.github/ISSUE_TEMPLATE/spec.yml`); write those + specs before the code, see them fail for the right reason, then make them green. The PR body lists each spec path, the command and the assertion that failed before the change, then the green run after it, + or repeats the issue's `No test-first: ` - Update tests if modifying existing code - Mark PR as WIP if not ready - Include description, screenshots/videos, and link to issues