mirror of
https://github.com/actualbudget/actual.git
synced 2026-07-15 14:43:36 -05:00
[AI] Add Claude Code skills for docs, commits, and PR review (#7967)
* [AI] Add Claude Code skills for docs, commits, and PR review Add three project-scoped skills under .claude/skills/ so any contributor using Claude Code in this repo gets the same conventions applied automatically: writing-actual-docs (points Claude at writing-docs.md before any docs work), committing-actual-changes (points Claude at pr-and-commit-rules.md before any commit/PR so the [AI] prefix and template-blank rule are followed), and review-actual-pr (end-to-end offline PR review with browser testing via playwright-cli, never posts to GitHub). All skill paths and tool assumptions are repo-relative (resolved via git rev-parse --show-toplevel) so they work on any contributor's machine, not just the author's. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [AI] Fix remaining hardcoded $HOME path in PR-review playbook The Step 3 highlight-overlay example still referenced $HOME/.claude/skills/review-actual-pr/... which only resolves on the original author's machine. Switch to the same git rev-parse --show-toplevel pattern used elsewhere in the skill so it works for any contributor with a checkout of the repo. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [AI] Add release note for #7967 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * [AI] Drop "add AI generated label" instruction now that it auto-applies The "AI generated" PR label is now applied automatically by GitHub Actions to any PR whose title starts with [AI], so the manual "add this label" instruction is stale. It was also misleading for outside contributors, who cannot apply labels on PRs against this repo regardless. Updated the canonical rules file, the committing-actual-changes skill, and the review-actual-pr code-review rubric to describe the prefix as the single trigger and note that the label follows automatically. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Update Claude Code skills description Simplified the description of Claude Code skills. * [AI] Allow cat and mv in PR-review skill's tool allowlist The workflow saves the final report via `cat > review.md <<'EOF'` (step 6) and rotates a stale prior report via `mv` (step 2, re-runs), but the allowed-tools line did not permit either, so those commands would prompt or fail at runtime. Add Bash(cat:*) and Bash(mv:*) — keeping the allowlist tight rather than broadening to Bash(*), so unrelated shell commands still require explicit authorization. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
committed by
GitHub
parent
5565236ba7
commit
64f230833c
27
.claude/skills/committing-actual-changes/SKILL.md
Normal file
27
.claude/skills/committing-actual-changes/SKILL.md
Normal file
@@ -0,0 +1,27 @@
|
||||
---
|
||||
name: committing-actual-changes
|
||||
description: Use whenever creating, drafting, finalizing, or amending a git commit or a pull request in the Actual Budget repo (actualbudget/actual) — including phrases like "commit this", "make a commit", "stage and commit", "open a PR", "create a pull request", "send this for review", "push these changes", "ship it", "submit a PR", "raise a PR", or any time finishing implementation work in this repo where committing or opening a PR is the natural next step. Also trigger when the user mentions the `[AI]` prefix, the PR template, git hooks, or `--no-verify` in this repo. The repo enforces strict, non-obvious rules for AI-generated contributions (mandatory `[AI]` prefix on every commit message and PR title, leaving the PR template blank, never skipping hooks, specific git-safety constraints, a pre-commit quality checklist) and not following them produces commits and PRs that have to be redone or manually fixed by a maintainer.
|
||||
---
|
||||
|
||||
# Committing Changes in actualbudget/actual
|
||||
|
||||
Before creating any git commit or pull request in this repo, read:
|
||||
|
||||
**`.github/agents/pr-and-commit-rules.md`**
|
||||
|
||||
It is the authoritative source for the `[AI]` prefix requirement, the git-safety rules, the pre-commit quality checklist (`yarn typecheck`, `yarn lint:fix`, relevant tests, translated strings), the rule about leaving the PR template blank, and the quick-reference workflow. Read it on every commit/PR session so any updates to those rules are picked up automatically.
|
||||
|
||||
## The one rule worth restating
|
||||
|
||||
Every commit message and every pull request title MUST begin with `[AI]`. This is the single most-violated rule and the cost of forgetting it is high — the commit has to be amended or the PR has to be renamed by hand. Examples:
|
||||
|
||||
- `[AI] Fix type error in account validation` — correct
|
||||
- `Fix type error in account validation` — wrong, missing prefix
|
||||
|
||||
Everything else (git safety, hooks, label, PR template, pre-commit checklist) lives in the rules file. Read it before each commit/PR rather than relying on this skill's summary, because the rules file evolves and this skill deliberately does not restate it.
|
||||
|
||||
## Why this matters
|
||||
|
||||
Maintainers triage AI-authored contributions separately, and the `[AI]` prefix is what makes that triage fast — it also drives the automatic `"AI generated"` PR label, so getting the prefix right is the one action that gates the whole triage path. Without it the PR looks like a normal human contribution and the wrong review process gets applied. The "do not fill in the PR template" rule exists because the human is the one who actually tested the change and can write the Description / Testing / Checklist sections honestly — an AI-filled template misrepresents who did what. The hook-skipping prohibition (`--no-verify`, `--no-gpg-sign`) exists because this codebase enforces formatting and lint at commit time deliberately; bypassing the hook is how broken code lands on a branch.
|
||||
|
||||
The rules file is short. Read it.
|
||||
217
.claude/skills/review-actual-pr/SKILL.md
Normal file
217
.claude/skills/review-actual-pr/SKILL.md
Normal file
@@ -0,0 +1,217 @@
|
||||
---
|
||||
name: review-actual-pr
|
||||
description: Comprehensive review of a pull request in the actualbudget/actual repo. Performs an offline code review against the repo's CODE_REVIEW_GUIDELINES.md and AGENTS.md (no GitHub comments are ever posted), then browser-tests the change with playwright-cli. For bug PRs, reproduces the issue on edge.actualbudget.org first and verifies the fix on the Netlify preview; for feature/enhancement PRs, exercises the change on the preview and captures annotated screenshots highlighting the new functionality. Use whenever the user asks to review, test, validate, vet, sanity-check, QA, or otherwise look at a PR / pull request in the Actual Budget repo — including phrases like "review #1234", "check this PR", "test the fix in 1234", "does PR 1234 work", "validate PR 1234", or when given a github.com/actualbudget/actual/pull/N URL. Trigger even when the user doesn't explicitly say "review".
|
||||
allowed-tools: Bash(gh:*) Bash(playwright-cli:*) Bash(npx:*) Bash(mkdir:*) Bash(ls:*) Bash(git:*) Bash(cat:*) Bash(mv:*) Read Edit Write
|
||||
---
|
||||
|
||||
# Review an Actual Budget PR
|
||||
|
||||
This skill reviews a pull request in `actualbudget/actual` end-to-end: a thorough code review against the repo's documented rules, plus a browser-based functional test using `playwright-cli`. **It never posts anything back to GitHub.** All findings come back to the chat as suggested changes the user can apply themselves.
|
||||
|
||||
## Why this skill exists
|
||||
|
||||
Reviewing a PR thoroughly involves three things that are tedious to do by hand: reading the diff against the repo's many style/correctness rules, opening the right Netlify preview, and (for bugs) confirming the same scenario on the live edge build to prove the fix actually changes behavior. This skill collapses that into one flow.
|
||||
|
||||
## Hard rules
|
||||
|
||||
These rules exist because violating them defeats the purpose of an offline review or causes harm:
|
||||
|
||||
- **Never** run `gh pr comment`, `gh pr review`, `gh api ... /comments`, or anything else that writes to the PR thread. Output goes to the chat only.
|
||||
- **Never** push commits, create branches, or modify the working tree of the repo. Reading is fine; writing is not.
|
||||
- **Never** fabricate a reproduction. If the demo budget doesn't have the data needed to reproduce a bug, surface that to the user and stop — partial evidence is worse than no evidence.
|
||||
- Before running anything that touches GitHub, check `git config user.name` / `user.email`; if the identity isn't yours (e.g. a CI bot or shared account), be doubly careful that no command in this skill could write to the PR — comments from the wrong identity are worse than no comments.
|
||||
|
||||
## Workflow
|
||||
|
||||
### 1. Resolve input → PR number
|
||||
|
||||
Accept any of: `7756`, `#7756`, `actualbudget/actual#7756`, or a full `https://github.com/actualbudget/actual/pull/7756` URL. Strip to the integer PR number. The repo is always `actualbudget/actual`.
|
||||
|
||||
### 2. Fetch metadata + diff
|
||||
|
||||
```bash
|
||||
mkdir -p ~/Downloads/pr-review-<num>
|
||||
gh pr view <num> --repo actualbudget/actual \
|
||||
--json number,title,body,labels,state,isDraft,headRefName,headRepository,baseRefName,files,url,author,additions,deletions \
|
||||
> ~/Downloads/pr-review-<num>/pr.json
|
||||
gh pr diff <num> --repo actualbudget/actual \
|
||||
> ~/Downloads/pr-review-<num>/diff.patch
|
||||
```
|
||||
|
||||
If the PR is `closed` or `merged`, tell the user and ask whether to continue (the preview URL may still work for merged PRs for a while). If `isDraft` is true, note it but proceed.
|
||||
|
||||
**Re-runs.** If `~/Downloads/pr-review-<num>/review.md` already exists from a previous run, read it before doing anything else. Compare its `head SHA` (recorded in the report header — see step 6) against the current PR head:
|
||||
|
||||
```bash
|
||||
gh pr view <num> --repo actualbudget/actual --json headRefOid -q .headRefOid
|
||||
```
|
||||
|
||||
- If the SHA matches, the previous review is still valid — surface a one-paragraph summary of what changed since (nothing, in this case) and ask the user whether to re-run testing, re-run code review, or just reprint the prior report. Don't redo work the user didn't ask for.
|
||||
- If the SHA differs, fetch the diff between the old and new head (`git fetch origin pull/<num>/head` if needed, then `git diff <old-sha> <new-sha>`) so the new review can call out what changed since last time. Move the previous report to `review.<old-sha-short>.md` so it's preserved as history rather than overwritten.
|
||||
|
||||
### 3. Classify: bug vs feature/enhancement
|
||||
|
||||
Auto-detect using these signals, in order of strength:
|
||||
|
||||
- **Bug** if any label name matches (case-insensitive) `bug`, `defect`, `regression`, OR title starts with `fix:`, `bug:`, `hotfix:`, `[AI] fix`, `[AI] bug`, OR body contains `Fixes #` / `Closes #` linked to an issue with a `bug` label.
|
||||
- **Feature / enhancement** if any label matches `enhancement`, `feature`, `feature request`, `improvement`, OR title starts with `feat:`, `feature:`, `enhancement:`, `[AI] add`, `[AI] feat`, OR the diff adds substantial new components/pages.
|
||||
- **Other** (refactor, chore, docs, deps): tell the user the PR doesn't look like a user-facing change, do the code review only, skip browser testing unless the user insists.
|
||||
|
||||
If both signals fire (rare) or neither fires confidently, ask via `AskUserQuestion`. Don't guess.
|
||||
|
||||
### 4. Code review
|
||||
|
||||
Read `references/code-review-rubric.md` first — it's the condensed rule set. Then walk the diff and produce findings in three tiers:
|
||||
|
||||
- **Critical** — bugs, security issues, broken types/build, data loss risk, anything that would harm users.
|
||||
- **Important** — repo-rule violations: new `@ts-strict-ignore`, new `eslint-disable` / `oxlint-disable`, untranslated user-facing strings, new UI-tweak settings, `as` where `satisfies` would work, missing `FinancialText`/`tnum` on standalone financial numbers, `any`/`unknown` without justification, wrong import sources (e.g. `useNavigate` from react-router instead of `src/hooks`), `[AI]` prefix missing from PR title.
|
||||
- **Suggestions** — clarity, naming, dead code, repeated patterns worth extracting, unnecessary mocks in tests.
|
||||
|
||||
For every finding, include:
|
||||
|
||||
- `path:line` (or `path:line-line` for ranges) — pulled from the diff hunks, not approximated.
|
||||
- One- or two-sentence description of the problem.
|
||||
- A concrete suggested change — actual replacement code or a unified-diff snippet — that the user can paste. Don't give vague advice like "consider refactoring"; give the refactor.
|
||||
|
||||
If the PR has zero findings in a tier, say so explicitly ("no Critical issues found") rather than omitting the heading. The user wants to see that the dimension was checked.
|
||||
|
||||
### 5. Browser testing
|
||||
|
||||
Branch on classification.
|
||||
|
||||
#### Bug PR
|
||||
|
||||
Follow `references/browser-testing-bug.md`. The shape is:
|
||||
|
||||
1. Open `https://edge.actualbudget.org/`, do the standard demo setup, reproduce the bug per the PR/issue description, save `~/Downloads/pr-review-<num>/before-edge.png`.
|
||||
2. Open `https://deploy-preview-<num>.demo.actualbudget.org/`, do the same setup, attempt the same repro, save `~/Downloads/pr-review-<num>/after-preview.png`.
|
||||
3. State plainly:
|
||||
- `Bug reproduces on edge: yes/no`
|
||||
- `Bug reproduces on preview: yes/no`
|
||||
- `Verdict: fix confirmed / fix not visible / repro inconclusive`
|
||||
|
||||
If the preview URL doesn't load (404, 502, build still running), wait briefly and retry once; if it still fails, report it and stop the testing phase. The code review still ships.
|
||||
|
||||
#### Feature / enhancement PR
|
||||
|
||||
Follow `references/browser-testing-feature.md`. The shape is:
|
||||
|
||||
1. Open `https://deploy-preview-<num>.demo.actualbudget.org/` only — do **not** also open edge for features.
|
||||
2. Do the standard demo setup.
|
||||
3. Derive an exploration plan from the PR title + body (what's new, where in the UI, what user-visible behavior).
|
||||
4. Walk through the feature step by step. At each meaningful step, take an annotated screenshot — use `references/highlight-element.js` (described below) to outline the new UI before screenshotting.
|
||||
5. Take one final clean (un-annotated) screenshot of the feature in its end state, so the user has a presentable reference too.
|
||||
|
||||
All screenshots go to `~/Downloads/pr-review-<num>/`.
|
||||
|
||||
### 6. Output the report
|
||||
|
||||
Build the report as one markdown document with this exact shape:
|
||||
|
||||
```
|
||||
# PR #<num>: <title>
|
||||
|
||||
**Author:** <author> · **Classification:** <bug | feature | other> · **Diff:** +<additions> / −<deletions>
|
||||
**Head SHA:** <full sha> · **Reviewed:** <ISO 8601 timestamp>
|
||||
|
||||
## Code review
|
||||
|
||||
### Critical
|
||||
- ...
|
||||
|
||||
### Important
|
||||
- ...
|
||||
|
||||
### Suggestions
|
||||
- ...
|
||||
|
||||
## Testing
|
||||
|
||||
<narrative — what was reproduced/exercised, on which environment, what happened>
|
||||
|
||||
**Screenshots:**
|
||||
- `~/Downloads/pr-review-<num>/<file>.png` — <one-line caption>
|
||||
|
||||
**Verdict:** <one line>
|
||||
|
||||
---
|
||||
|
||||
_No comments were posted to GitHub. All suggestions above are local recommendations._
|
||||
```
|
||||
|
||||
The `Head SHA` and `Reviewed` lines are mandatory — they're what makes re-runs (step 2) able to detect staleness. The trailing offline-review line is mandatory too.
|
||||
|
||||
**Save the report and print it:**
|
||||
|
||||
```bash
|
||||
# Save first so it survives anything that happens to the chat
|
||||
cat > ~/Downloads/pr-review-<num>/review.md <<'EOF'
|
||||
<the full report markdown>
|
||||
EOF
|
||||
```
|
||||
|
||||
Then print the same content to chat. Saving is non-negotiable — the whole point is the user can come back to `~/Downloads/pr-review-<num>/` later and find the screenshots, the diff, the metadata, and the review notes that explain them, all in one place.
|
||||
|
||||
If you have structured intermediate data worth keeping (e.g., parsed findings as a list), also drop it as `~/Downloads/pr-review-<num>/findings.json` — it's optional but cheap and useful for any tooling the user wires up later.
|
||||
|
||||
The folder layout after a successful run looks like:
|
||||
|
||||
```
|
||||
~/Downloads/pr-review-<num>/
|
||||
├── pr.json # gh pr view output
|
||||
├── diff.patch # gh pr diff output
|
||||
├── review.md # the markdown report
|
||||
├── findings.json # optional structured findings
|
||||
├── before-edge.png # bug PRs only
|
||||
├── after-preview.png # bug PRs only
|
||||
├── feature-step-N.png # feature PRs, annotated
|
||||
└── feature-final.png # feature PRs, clean
|
||||
```
|
||||
|
||||
## Standard demo setup
|
||||
|
||||
For both edge and preview testing, the first-run flow is:
|
||||
|
||||
1. Wait for the setup screen to render.
|
||||
2. Click **"Don't use a server"**.
|
||||
3. Click **"View demo"**.
|
||||
|
||||
This loads a populated test budget with realistic accounts, transactions, categories, and budgeted amounts — far more useful than starting empty (per `AGENTS.md` "Testing and previewing the app"). The demo data is the same on edge and preview, so reproductions translate directly.
|
||||
|
||||
If the page already shows a budget (state was preserved from a previous session), use it as-is. If you need a fresh state, clear localStorage:
|
||||
|
||||
```bash
|
||||
playwright-cli localstorage-clear
|
||||
playwright-cli reload
|
||||
```
|
||||
|
||||
## Highlighting elements in screenshots
|
||||
|
||||
For feature PRs, use `references/highlight-element.js` via `playwright-cli run-code` to draw a red dashed bounding box and a label callout over the new UI before screenshotting. Pass the selector and label as environment-style arguments.
|
||||
|
||||
Resolve the script path from the repo root so this works for any contributor:
|
||||
|
||||
```bash
|
||||
SKILL_DIR="$(git rev-parse --show-toplevel)/.claude/skills/review-actual-pr"
|
||||
|
||||
SELECTOR='[data-testid="new-thing"]' LABEL='New filter chip' \
|
||||
playwright-cli run-code --filename="$SKILL_DIR/references/highlight-element.js"
|
||||
playwright-cli screenshot --filename="$HOME/Downloads/pr-review-<num>/feature-step-1.png"
|
||||
# remove overlay before next step so unrelated screenshots stay clean
|
||||
playwright-cli eval "document.querySelectorAll('[data-pr-highlight]').forEach(n => n.remove())"
|
||||
```
|
||||
|
||||
If you can't find a stable selector for the new UI, use a CSS selector that matches what's actually in the DOM after a `playwright-cli snapshot` — don't fall back to silently un-annotated screenshots, since highlighting is the point of feature shots.
|
||||
|
||||
## Tooling assumptions
|
||||
|
||||
- `gh` is authenticated for read access to `actualbudget/actual`. A read-only identity (bot, CI, or personal account) is fine — see the hard rule above about not writing back.
|
||||
- `playwright-cli` is on PATH (typically installed via the `playwright-cli` skill). If it isn't, fall back to `npx --no-install playwright-cli`; if that also fails, surface it and stop the browser-testing phase rather than guessing at an alternative tool.
|
||||
- Output goes to `~/Downloads/pr-review-<num>/` (exists on macOS, Linux, and Windows). If a contributor prefers a different location, they can override it — but keep everything for one run in a single directory so the report and its screenshots stay together.
|
||||
|
||||
## Reference files
|
||||
|
||||
- `references/code-review-rubric.md` — condensed rules from `CODE_REVIEW_GUIDELINES.md` + `AGENTS.md`. Read before reviewing the diff.
|
||||
- `references/browser-testing-bug.md` — step-by-step playbook for bug PRs (edge → preview).
|
||||
- `references/browser-testing-feature.md` — step-by-step playbook for feature PRs (preview only, annotated).
|
||||
- `references/highlight-element.js` — overlay script for annotated screenshots.
|
||||
@@ -0,0 +1,85 @@
|
||||
# Browser testing playbook: bug PR
|
||||
|
||||
For bug PRs we want **two pieces of evidence**: that the bug existed before the fix (reproduce on `edge.actualbudget.org`), and that it doesn't anymore (try the same repro on the PR's Netlify preview). Both are needed — a fix that "looks right" in code but doesn't actually change runtime behavior is a regression waiting to happen, and a bug that doesn't reproduce on edge means the fix may be addressing a phantom.
|
||||
|
||||
## Constants
|
||||
|
||||
- `EDGE_URL = https://edge.actualbudget.org/`
|
||||
- `PREVIEW_URL = https://deploy-preview-<num>.demo.actualbudget.org/` (substitute the PR number)
|
||||
- Output dir: `~/Downloads/pr-review-<num>/`
|
||||
|
||||
## Step 1 — derive a reproduction plan from the PR
|
||||
|
||||
From the PR title, body, and any linked issue, write a short ordered list of steps to trigger the bug. Be concrete: "Open Reports → click Cash Flow → set date range to last month → expect chart, observe blank canvas." If the PR doesn't say enough to derive a repro, fetch the linked issue:
|
||||
|
||||
```bash
|
||||
# extract issue number from "Fixes #1234" / "Closes #1234"
|
||||
ISSUE_NUM=...
|
||||
gh issue view $ISSUE_NUM --repo actualbudget/actual --json title,body,labels
|
||||
```
|
||||
|
||||
If after reading the linked issue you still don't have a clear repro, **stop the testing phase and tell the user**. Don't make one up.
|
||||
|
||||
## Step 2 — reproduce on edge
|
||||
|
||||
```bash
|
||||
playwright-cli open $EDGE_URL
|
||||
playwright-cli snapshot
|
||||
```
|
||||
|
||||
Standard demo setup (per `AGENTS.md`):
|
||||
|
||||
1. Click "Don't use a server".
|
||||
2. Click "View demo".
|
||||
|
||||
Wait for the budget to load. Then walk the repro plan one step at a time, taking a snapshot after each step so the next click has fresh element refs. When you hit the failure state, capture it:
|
||||
|
||||
```bash
|
||||
playwright-cli screenshot --filename=$HOME/Downloads/pr-review-<num>/before-edge.png
|
||||
```
|
||||
|
||||
Note in your running summary: did the bug reproduce as described? If not, that's an important finding — say so in the report and stop. The fix may be defensive, addressing a path that's hard to hit, in which case escalate to the user with what you saw.
|
||||
|
||||
If the demo data doesn't have what's needed (e.g., the bug requires a multi-currency setup and demo is single-currency), surface that and stop. Don't fabricate.
|
||||
|
||||
## Step 3 — verify on preview
|
||||
|
||||
Close edge and open the preview:
|
||||
|
||||
```bash
|
||||
playwright-cli close
|
||||
playwright-cli open $PREVIEW_URL
|
||||
playwright-cli snapshot
|
||||
```
|
||||
|
||||
If the page returns a 404 or "site not found", wait ~10 seconds and reload once — Netlify deploys can lag the GitHub PR event. If it's still broken, stop testing, report the preview URL didn't load, and ship the code review without the testing section. Don't try to test against a stale build.
|
||||
|
||||
Repeat the demo setup and the same repro steps. Capture the result:
|
||||
|
||||
```bash
|
||||
playwright-cli screenshot --filename=$HOME/Downloads/pr-review-<num>/after-preview.png
|
||||
```
|
||||
|
||||
## Step 4 — verdict
|
||||
|
||||
State three things plainly in the report's Testing section:
|
||||
|
||||
- `Bug reproduces on edge: yes / no`
|
||||
- `Bug reproduces on preview: yes / no`
|
||||
- `Verdict: fix confirmed | fix not observable | repro inconclusive`
|
||||
|
||||
Truth table:
|
||||
|
||||
| Edge | Preview | Verdict |
|
||||
| ---- | ------- | ------------------------------------- |
|
||||
| yes | no | **fix confirmed** |
|
||||
| yes | yes | **fix not observable** — escalate |
|
||||
| no | no | **repro inconclusive** — describe why |
|
||||
| no | yes | **regression** — the PR made it worse |
|
||||
|
||||
The "regression" case is rare but important; if it ever fires, mark it Critical in the code review section too.
|
||||
|
||||
## Notes
|
||||
|
||||
- If the bug is visual (layout, color, alignment), prefer the same viewport on both URLs: `playwright-cli resize 1280 800` before each setup. Otherwise differing default sizes can mask or fake the difference.
|
||||
- For bugs in transaction lists or reports, the demo budget's data may be too small to show the issue. If you suspect that, say so — don't pad with "looks fine" when you didn't actually exercise the code path.
|
||||
@@ -0,0 +1,95 @@
|
||||
# Browser testing playbook: feature / enhancement PR
|
||||
|
||||
For feature PRs we don't compare against edge — the feature didn't exist there. We just need to demonstrate, on the PR preview, that the new behavior works as advertised, and produce screenshots that **make the new functionality obvious**. Annotated shots beat raw shots: a red dashed box around the new chip/menu/setting tells the next reviewer where to look without reading any caption.
|
||||
|
||||
## Constants
|
||||
|
||||
- `PREVIEW_URL = https://deploy-preview-<num>.demo.actualbudget.org/`
|
||||
- Output dir: `~/Downloads/pr-review-<num>/`
|
||||
- Highlight script: `$(git rev-parse --show-toplevel)/.claude/skills/review-actual-pr/references/highlight-element.js`
|
||||
|
||||
## Step 1 — derive an exploration plan
|
||||
|
||||
From the PR title and body, list:
|
||||
|
||||
1. **What's new** in user terms (one sentence).
|
||||
2. **Where it lives** in the UI (page / panel / menu).
|
||||
3. **The path a user takes to use it** (3–8 steps).
|
||||
4. **The end state** that proves it works.
|
||||
|
||||
If the PR description is sparse and you can't write a clear plan, ask the user before testing — there's no point screenshotting a feature you don't understand.
|
||||
|
||||
## Step 2 — open the preview and set up
|
||||
|
||||
```bash
|
||||
playwright-cli open $PREVIEW_URL
|
||||
playwright-cli snapshot
|
||||
```
|
||||
|
||||
Standard demo setup:
|
||||
|
||||
1. Click "Don't use a server".
|
||||
2. Click "View demo".
|
||||
|
||||
Wait for the budget to load. Set a sensible viewport so screenshots are presentable:
|
||||
|
||||
```bash
|
||||
playwright-cli resize 1440 900
|
||||
```
|
||||
|
||||
## Step 3 — walk the feature, screenshotting with annotations
|
||||
|
||||
For each meaningful step in the plan:
|
||||
|
||||
1. Navigate / click / fill as needed (use refs from `playwright-cli snapshot`).
|
||||
2. Take a snapshot to find a stable selector for the _new_ UI element introduced by the PR. Prefer `data-testid` if present, then a role + accessible name, then a CSS selector. Avoid brittle nth-child chains.
|
||||
3. Inject the highlight overlay (resolve the script path via `git rev-parse` so it works for any contributor):
|
||||
```bash
|
||||
SKILL_DIR="$(git rev-parse --show-toplevel)/.claude/skills/review-actual-pr"
|
||||
SELECTOR='<your selector>' LABEL='<short label, e.g. "New filter chip">' \
|
||||
playwright-cli run-code --filename="$SKILL_DIR/references/highlight-element.js"
|
||||
```
|
||||
4. Screenshot:
|
||||
```bash
|
||||
playwright-cli screenshot --filename=$HOME/Downloads/pr-review-<num>/feature-step-N.png
|
||||
```
|
||||
5. Remove the overlay so it doesn't bleed into the next shot:
|
||||
```bash
|
||||
playwright-cli eval "document.querySelectorAll('[data-pr-highlight]').forEach(n => n.remove())"
|
||||
```
|
||||
|
||||
If a step _changes the layout_ (modal opens, panel slides out), re-snapshot first — refs change.
|
||||
|
||||
## Step 4 — final clean shot
|
||||
|
||||
After the last annotated step, take one more screenshot **without** any overlay, in the feature's end state:
|
||||
|
||||
```bash
|
||||
playwright-cli screenshot --filename=$HOME/Downloads/pr-review-<num>/feature-final.png
|
||||
```
|
||||
|
||||
This gives the user a presentable shot to drop into release notes / PR descriptions later, separate from the QA evidence shots.
|
||||
|
||||
## Step 5 — verdict and report
|
||||
|
||||
State plainly:
|
||||
|
||||
- `Behavior matches PR description: yes / no / partially`
|
||||
- For each step: a one-line note about what happened and which screenshot shows it.
|
||||
- If anything in the PR description isn't actually visible in the build, call it out — the PR may need further work.
|
||||
|
||||
## When highlighting fails
|
||||
|
||||
If you can't find a stable selector for the new UI:
|
||||
|
||||
1. Take an unannotated snapshot first to confirm the element is on screen.
|
||||
2. Try a coarser selector (the parent container).
|
||||
3. As a last resort, skip annotation for that step but still take the screenshot, and note in the report which step lacks a highlight and why.
|
||||
|
||||
Never silently produce un-annotated shots when the user asked for highlighted ones — flag the gap so they know.
|
||||
|
||||
## Things to avoid
|
||||
|
||||
- **Don't** also test on edge. The feature isn't there. Comparing isn't useful.
|
||||
- **Don't** clear localStorage between steps unless a step requires fresh state — it logs you out of the demo and forces redoing setup.
|
||||
- **Don't** assume animations have finished. After a `click` that opens a panel, take a snapshot before screenshotting to make sure the post-animation state is what's captured.
|
||||
@@ -0,0 +1,82 @@
|
||||
# Code review rubric for actualbudget/actual
|
||||
|
||||
A condensed checklist distilled from `CODE_REVIEW_GUIDELINES.md`, `AGENTS.md`, and `.github/agents/pr-and-commit-rules.md` so the reviewer can stay in flow without re-reading hundreds of lines mid-review. When a finding fires, cite the rule by section name (e.g., "Type assertions") so the user can trace it back.
|
||||
|
||||
## Hard rejections (Critical or Important)
|
||||
|
||||
These come from `CODE_REVIEW_GUIDELINES.md` and are non-negotiable unless the PR contains a documented justification.
|
||||
|
||||
- **New settings for UI tweaks.** Actual deliberately resists settings bloat. If the PR adds a user-facing toggle/preference for something a theme/design token could express, flag as Important and propose the theme-based alternative.
|
||||
- **New `@ts-strict-ignore` comments.** Strict checking exists for a reason; suppressions undermine it. Suggest fixing the underlying types instead. Mark as Important (Critical if it's hiding a real type error).
|
||||
- **New `eslint-disable` or `oxlint-disable` comments.** Same logic. Propose the fix that satisfies the rule.
|
||||
- **Secrets / credentials in code.** Critical. Always.
|
||||
|
||||
## Type rules (`AGENTS.md` + `CODE_REVIEW_GUIDELINES.md`)
|
||||
|
||||
- Prefer `type` over `interface`.
|
||||
- No `enum` — use object maps.
|
||||
- No `any` / `unknown` without justification. Look in `packages/loot-core/src/types/` for an existing type before suggesting a new one.
|
||||
- Prefer `satisfies SomeType` over `as SomeType`. The exception is genuine runtime type guards — those need a comment explaining why `as` is safe.
|
||||
- No `React.FC`, `React.FunctionComponent`, or `React.*` general usage — use named imports and type props directly.
|
||||
|
||||
## React patterns
|
||||
|
||||
- React Compiler is on in `desktop-client`. Don't add manual `useCallback` / `useMemo` / `React.memo` unless a non-compiled dependency genuinely needs a stable identity. Flag unnecessary memoization as a Suggestion.
|
||||
- Use `<Link>` from the router, not `<a>` tags.
|
||||
- Hooks must come from the project's wrappers:
|
||||
- `useNavigate`, etc. from `src/hooks` (not `react-router-dom`).
|
||||
- `useDispatch`, `useSelector`, `useStore` from `src/redux` (not `react-redux`).
|
||||
- Avoid nested component definitions inside other components (unstable identities).
|
||||
|
||||
## Imports
|
||||
|
||||
- `import { v4 as uuidv4 } from 'uuid';` — never the default `uuid` import.
|
||||
- No direct color imports; use the theme.
|
||||
- No `@actual-app/web/*` imports inside `loot-core`.
|
||||
- Import order: React → built-ins → external → actual packages (`loot-core`, `@actual-app/components`) → parent → sibling → index. Keep blank lines between groups. Lint enforces this; flag violations as Suggestions.
|
||||
|
||||
## Internationalization
|
||||
|
||||
- All user-facing strings must be translated. Prefer `<Trans>` over `t()`.
|
||||
- The `actual/no-untranslated-strings` ESLint rule catches most of this, but the reviewer should still spot-check obvious misses (e.g., `<Button>Save</Button>`).
|
||||
|
||||
## Financial typography
|
||||
|
||||
- Standalone financial numbers must be wrapped in `FinancialText`, or have `styles.tnum` applied directly when wrapping isn't possible. Tabular figures matter for legibility in budget UIs.
|
||||
|
||||
## Tests
|
||||
|
||||
- Minimize mocks. Real implementations > stubs for unit and component tests. Only mock genuinely impractical dependencies (external network, filesystem in unit context).
|
||||
- Vitest globals (`describe`, `it`, `expect`, `beforeEach`) are fine — no need to import them.
|
||||
- E2E lives in `packages/desktop-client/e2e/`. Page models in `e2e/page-models/` should be reused, not duplicated.
|
||||
|
||||
## Platform-specific code
|
||||
|
||||
- No direct `.api` or `.electron` imports from non-platform code. Use the conditional exports in `loot-core`.
|
||||
|
||||
## Commit / PR rules (from `.github/agents/pr-and-commit-rules.md`)
|
||||
|
||||
These are mandatory for AI-authored PRs. Verify and flag misses as Important.
|
||||
|
||||
- Commit messages prefixed with `[AI]`.
|
||||
- PR title prefixed with `[AI]` (the `"AI generated"` label is auto-applied based on this prefix, so no need to verify it separately).
|
||||
- PR template **not** filled in (unless a human explicitly asked for it, in which case it must be in Chinese — yes, really).
|
||||
- No `--no-verify`, `--no-gpg-sign`, force-pushes to main, or destructive git ops.
|
||||
|
||||
## Specific files / paths to give extra scrutiny
|
||||
|
||||
- `packages/loot-core/src/server/migrations/` — schema migrations. Must be idempotent; flag any that aren't, and any that drop or rewrite data without a backfill story. **Critical** by default.
|
||||
- `packages/loot-core/src/server/budget/` — budget math. Off-by-one or rounding errors here directly cost users money in their reports. Read carefully.
|
||||
- `packages/desktop-client/src/components/budget/` — main UI surface. Re-render patterns and selector usage matter here.
|
||||
- `packages/sync-server/` — server-side; review CRDT / sync changes carefully for ordering and race conditions.
|
||||
- `packages/desktop-client/e2e/*-snapshots/` — VRT snapshots. If the PR updates these, the diff itself is the review: open the new PNG, confirm the visual change matches the PR's stated intent.
|
||||
|
||||
## What to skip
|
||||
|
||||
- Generated files: `packages/component-library/src/icons/` (auto-generated; don't review).
|
||||
- Build artifacts: `*/dist`, `*/build`, `*/lib-dist` (shouldn't be in PRs).
|
||||
- Translation source files: don't review machine-generated translations word by word.
|
||||
|
||||
## Output format reminder
|
||||
|
||||
Every finding gets: `path:line`, problem in 1–2 sentences, and a concrete suggested change (replacement code or diff snippet). No vague advice. The user wants to paste the fix.
|
||||
@@ -0,0 +1,93 @@
|
||||
// Draw a red dashed bounding box + label over an element so the next
|
||||
// playwright-cli screenshot captures a "look here" annotation.
|
||||
//
|
||||
// Inputs come via env vars when invoked through:
|
||||
// SELECTOR='...' LABEL='...' playwright-cli run-code --filename=this-file.js
|
||||
//
|
||||
// The overlay nodes are tagged with data-pr-highlight so they can be wiped:
|
||||
// playwright-cli eval "document.querySelectorAll('[data-pr-highlight]').forEach(n => n.remove())"
|
||||
|
||||
module.exports = async page => {
|
||||
const selector = process.env.SELECTOR;
|
||||
const label = process.env.LABEL || '';
|
||||
|
||||
if (!selector) {
|
||||
throw new Error('SELECTOR env var is required');
|
||||
}
|
||||
|
||||
const result = await page.evaluate(
|
||||
({ selector, label }) => {
|
||||
const el = document.querySelector(selector);
|
||||
if (!el) return { ok: false, reason: 'not-found' };
|
||||
|
||||
el.scrollIntoView({
|
||||
block: 'center',
|
||||
inline: 'center',
|
||||
behavior: 'instant',
|
||||
});
|
||||
|
||||
const rect = el.getBoundingClientRect();
|
||||
if (rect.width === 0 || rect.height === 0) {
|
||||
return { ok: false, reason: 'zero-size' };
|
||||
}
|
||||
|
||||
const PAD = 6;
|
||||
const box = document.createElement('div');
|
||||
box.setAttribute('data-pr-highlight', 'box');
|
||||
Object.assign(box.style, {
|
||||
position: 'fixed',
|
||||
left: `${rect.left - PAD}px`,
|
||||
top: `${rect.top - PAD}px`,
|
||||
width: `${rect.width + PAD * 2}px`,
|
||||
height: `${rect.height + PAD * 2}px`,
|
||||
border: '3px dashed #e3342f',
|
||||
borderRadius: '6px',
|
||||
boxShadow: '0 0 0 9999px rgba(0,0,0,0.04)',
|
||||
pointerEvents: 'none',
|
||||
zIndex: '2147483647',
|
||||
boxSizing: 'border-box',
|
||||
});
|
||||
document.body.appendChild(box);
|
||||
|
||||
if (label) {
|
||||
const tag = document.createElement('div');
|
||||
tag.setAttribute('data-pr-highlight', 'label');
|
||||
tag.textContent = label;
|
||||
const labelTop = Math.max(0, rect.top - PAD - 28);
|
||||
Object.assign(tag.style, {
|
||||
position: 'fixed',
|
||||
left: `${rect.left - PAD}px`,
|
||||
top: `${labelTop}px`,
|
||||
padding: '4px 8px',
|
||||
background: '#e3342f',
|
||||
color: '#fff',
|
||||
fontFamily:
|
||||
'-apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, "Helvetica Neue", Arial, sans-serif',
|
||||
fontSize: '12px',
|
||||
fontWeight: '600',
|
||||
borderRadius: '4px',
|
||||
pointerEvents: 'none',
|
||||
zIndex: '2147483647',
|
||||
whiteSpace: 'nowrap',
|
||||
boxShadow: '0 1px 3px rgba(0,0,0,0.25)',
|
||||
});
|
||||
document.body.appendChild(tag);
|
||||
}
|
||||
|
||||
return {
|
||||
ok: true,
|
||||
rect: { x: rect.left, y: rect.top, w: rect.width, h: rect.height },
|
||||
};
|
||||
},
|
||||
{ selector, label },
|
||||
);
|
||||
|
||||
if (!result.ok) {
|
||||
throw new Error(
|
||||
`Highlight failed: ${result.reason} (selector: ${selector})`,
|
||||
);
|
||||
}
|
||||
|
||||
// Give the browser a frame to paint before the next screenshot fires.
|
||||
await page.waitForTimeout(50);
|
||||
};
|
||||
23
.claude/skills/writing-actual-docs/SKILL.md
Normal file
23
.claude/skills/writing-actual-docs/SKILL.md
Normal file
@@ -0,0 +1,23 @@
|
||||
---
|
||||
name: writing-actual-docs
|
||||
description: Use whenever creating, updating, editing, drafting, restructuring, or fixing documentation in the Actual Budget repo — specifically anything under `packages/docs/` (the Docusaurus site published at actualbudget.org/docs) or any change to `.md` / `.mdx` files in that package. Trigger for asks like "add a doc page for X", "update the FAQ", "write a guide for the new feature", "document this setting", "fix the docs about Y", "add a contributing page", or any work on the docs site, even when the user does not explicitly mention style, structure, or conventions. Actual's docs follow strict Docusaurus conventions (front matter, heading levels, image placement and naming, admonition syntax, Title Case, tone of voice, spelling allowlist) and writing them without consulting the project's style guide reliably produces output that fails review and needs to be redone.
|
||||
---
|
||||
|
||||
# Writing Actual Budget Documentation
|
||||
|
||||
Before writing or editing any documentation file under `packages/docs/`, read the project's style guide:
|
||||
|
||||
**`packages/docs/docs/contributing/writing-docs.md`**
|
||||
|
||||
It is the authoritative source for front matter, heading rules, folder structure, tone, Docusaurus admonitions and components, image placement and annotation, spelling allowlist, and naming standards — there is no point restating it here because it will go stale faster than the source.
|
||||
|
||||
## How to apply it
|
||||
|
||||
1. Read `writing-docs.md` in full before drafting.
|
||||
2. When the guide does not cover something, match the closest existing document in the same section rather than inventing a new pattern — site-wide consistency matters more than a marginally better local choice.
|
||||
3. For new screenshots, follow both the placement rule (`/static/img/<section>/<doc-prefix>-...png`) and the annotation guidance from the guide. Annotate any screenshot showing more than one element the reader needs to distinguish.
|
||||
4. Before declaring the work done, sanity-check the file against the guide's structural rules: exactly one H1, Title Case headings, no time-bound phrasing, images referenced from the correct path, and any new technical terms added to `.github/actions/spelling/allow/keywords.txt` so the spell-check bot passes.
|
||||
|
||||
## Why this matters
|
||||
|
||||
The docs are user-facing and the audience is mixed — many readers are not developers, so the guide deliberately favors verbose, step-by-step explanations over terse expert prose. The structural rules are what Docusaurus and the spell-check bot rely on, so getting them wrong breaks the build or the rendered sidebar. Reading the guide once at the start of the task is much cheaper than having a maintainer flag a dozen style issues in review.
|
||||
7
.github/agents/pr-and-commit-rules.md
vendored
7
.github/agents/pr-and-commit-rules.md
vendored
@@ -46,9 +46,7 @@ Before committing, ensure all of the following:
|
||||
- `[AI] Add support for new transaction categories`
|
||||
- `Fix type error in account validation` (MISSING PREFIX - NOT ALLOWED)
|
||||
|
||||
### Labels
|
||||
|
||||
Add the **"AI generated"** label to all AI-created pull requests. This helps maintainers understand the nature of the contribution.
|
||||
The `"AI generated"` label is applied automatically to any PR whose title starts with `[AI]`, so there is nothing extra to do — and outside contributors cannot apply labels anyway.
|
||||
|
||||
### PR Template: Do Not Fill In
|
||||
|
||||
@@ -65,6 +63,5 @@ Follow these steps when committing and creating PRs:
|
||||
4. Run relevant tests (`yarn test` for all, or workspace-specific)
|
||||
5. Stage files and commit with `[AI]` prefix — do not skip hooks
|
||||
6. When creating a PR:
|
||||
- Use `[AI]` prefix in the title
|
||||
- Add the `"AI generated"` label
|
||||
- Use `[AI]` prefix in the title (the `"AI generated"` label is auto-applied based on this prefix)
|
||||
- Leave the PR template blank (do not fill it in)
|
||||
|
||||
6
upcoming-release-notes/7967.md
Normal file
6
upcoming-release-notes/7967.md
Normal file
@@ -0,0 +1,6 @@
|
||||
---
|
||||
category: Maintenance
|
||||
authors: [MatissJanis]
|
||||
---
|
||||
|
||||
Add project-scoped Claude Code skills.
|
||||
Reference in New Issue
Block a user