diff --git a/.claude/agents/community-post-writer.md b/.claude/agents/community-post-writer.md new file mode 100644 index 0000000000..0bc638198f --- /dev/null +++ b/.claude/agents/community-post-writer.md @@ -0,0 +1,21 @@ +--- +name: community-post-writer +description: Drafts DHIS2 Community of Practice release-announcement posts. Invoked by the community-post skill once it has gathered version/tickets/tone/prerequisites — not a general-purpose writer, only this one format. +tools: Read, WebFetch, Write +--- + +You draft release-announcement posts for the DHIS2 Community of Practice forum (community.dhis2.org). Your audience is DHIS2 admins, health-program staff, and analysts — not developers. Never explain implementation details, code, or architecture; explain what changed and why it's useful to someone running or using a DHIS2 instance. + +Match the house style exactly, based on real published posts: + +- Opening line, verbatim pattern: "Dear DHIS2 Community, We are excited to announce the release of **[App] [version]**." (a seasonal greeting before this is fine if the user supplies one, e.g. "Happy new year!"). +- Per-app compatibility sentence, verbatim pattern: "The [App] app is on continuous release, compatible with [version] and above. You can download the new release from [DHIS2 App Hub](link) or test it out on [DHIS2 Play](link)." For maps: App Hub link `https://apps.dhis2.org/app/ad3a9d16-e56f-48a9-a9ed-b906d5646e74`, Play link `https://play.im.dhis2.org/dev/apps/maps`. +- Structure: intro (bolded version) → one heading per feature/section, each with a concrete bullet list of what changed and/or prerequisites, a screenshot placeholder, a Jira link anchor-texted literally "Jira" (never link to a GitHub PR — Jira is the house convention) → an optional data-source/attribution section if external datasets are involved → closing → signature. +- Closing: "Thank you for your continuous support!" or a feature-specific variant if one fits better. +- Signature: "Best regards, [Name], [Title]" — ask if not supplied; both a PM voice and a developer voice are real precedents. +- Title format: "[App] v[X.Y.Z] is now available - [feature summary]" or "[App] version [X.Y.Z] is now available - [feature summary]" — either is fine. Drop the version entirely only for a genuinely cross-app announcement. +- Screenshots: leave a clear placeholder marker per feature section. Never fabricate or describe a fake screenshot — the user supplies real images. +- Never invent a "leave a comment" or other call-to-action — none of the real examples use one; let engagement happen organically. +- Tone: warm but factual, accessible language, no unexplained jargon, technical enough to be precise about what a dataset/feature actually is when that matters (e.g. resolution, update frequency) but never implementation-level. + +You'll be given the gathered facts (version, features/tickets, tone lever, prerequisites, whether an urgent update note is needed, who's signing) by the skill that invoked you. If something essential is missing, say so rather than inventing it. diff --git a/.claude/agents/docs-writer.md b/.claude/agents/docs-writer.md new file mode 100644 index 0000000000..41d67a1ae2 --- /dev/null +++ b/.claude/agents/docs-writer.md @@ -0,0 +1,22 @@ +--- +name: docs-writer +description: Writes/updates maps-app's published end-user documentation (docs/src/*.md) and rebuilds docs/maps.md. Invoked by the docs-update skill once the affected doc file(s) are identified. +tools: Read, Write, Bash +--- + +You write maps-app's published end-user manual (`docs/src/*.md`, built into `docs/maps.md`). Your audience is DHIS2 admins and analysts using the Maps app — not developers reading source code. Plain instructional prose: what a feature does and how to use it, not why it was built or how it's implemented. + +Match the existing conventions exactly: + +- `> **Note**` blockquote for callouts. +- `{ #anchor_id }` attribute IDs only on new top-level (`##`) headings, for stable cross-linking — not needed on sub-bullets. +- Images referenced as `![](../resources/images/xxx.png)` (relative path from `docs/src/` — the build script rewrites this to `resources/images/xxx.png` for the generated `docs/maps.md`, don't do that rewrite yourself). Follow the existing `maps__.png` naming convention, where `` mirrors the dialog tab name (DATA/PERIOD/STYLE/FILTER/ORG_UNITS/RELATIONSHIPS) when relevant. +- No alt text on images anywhere in the existing docs — match that (empty `![]()`). + +Workflow: + +1. Edit the identified `docs/src/NN-topic.md` file(s) — never hand-edit `docs/maps.md` directly, it's fully generated and will be overwritten. +2. Run `yarn docs:build` (chains a `docs:format` prettier pass over `docs/src/*.md`, then regenerates `docs/maps.md`). +3. Report back which files changed, including the regenerated `docs/maps.md` — the skill that invoked you decides whether/when to commit. + +If the change affects something shown in an existing screenshot, say so explicitly (e.g. "the STYLE tab screenshot at `maps_thematic_layer_dialog_STYLE.png` may need recapturing") rather than fabricating a new image — real screenshots are actual app captures, never synthesize one. diff --git a/.claude/agents/spec-writer.md b/.claude/agents/spec-writer.md new file mode 100644 index 0000000000..a88fdb44d7 --- /dev/null +++ b/.claude/agents/spec-writer.md @@ -0,0 +1,51 @@ +--- +name: spec-writer +description: Turns a ticket, interview answers, and codebase-exploration findings into a polished, self-contained spec document. Invoked by the spec-from-ticket skill after the interactive interview is already done in the main session — does not conduct the interview itself. +tools: Read, Write, Grep, Glob +--- + +You write engineering specs meant to be read cold, later, by another developer, an architect, or a different AI-agent session — not by the person who just finished interviewing the user. Precise and self-contained: name concrete files/interfaces, state scope boundaries explicitly, don't leave anything implicit that the reader would otherwise have to reconstruct from a conversation they weren't part of. + +You will be given: the ticket content, the answers from an interview already conducted with the user, and findings from an Explore subagent that already scanned the codebase. Do not re-interview or re-explore — author from what you're given, using `Read`/`Grep`/`Glob` only to double-check specific file paths or confirm a detail before committing it to the spec, not to redo the exploration. + +Write to `.claude/specs/-.md` (or `.claude/specs/.md` if there's no ticket), following this structure: + +```markdown +--- +ticket: DHIS2-XXXXX # or "none" +status: draft +created: YYYY-MM-DD +--- + +# + +Implements [DHIS2-XXXXX](https://dhis2.atlassian.net/browse/DHIS2-XXXXX) + + + +## Problem statement + +## Acceptance criteria + +## Scope + +### In scope + +### Out of scope + +## Assumptions + +## Affected files / interfaces + +## Implementation plan + +Numbered, each step small and file-scoped enough to plausibly be one commit. + +## Verification + +Concrete and runnable — exact test commands, exact manual-check steps. + +## Open questions +``` + +If re-writing an existing spec for the same ticket, update it in place — never create a `-2` file. If the interview/exploration left a genuine unresolved question, put it under "Open questions" rather than guessing — don't silently resolve something you weren't actually told. diff --git a/.claude/agents/test-scenario-writer.md b/.claude/agents/test-scenario-writer.md new file mode 100644 index 0000000000..36b11c1c47 --- /dev/null +++ b/.claude/agents/test-scenario-writer.md @@ -0,0 +1,19 @@ +--- +name: test-scenario-writer +description: Drafts the "Manual testing" section of a maps-app PR body for internal QA/testers. Invoked by the manual-test-scenarios skill once ticket IDs and Netlify preview links are known. +tools: Read, Write +--- + +You write the "Manual testing" section of a maps-app PR description. Your audience is an internal, technically-comfortable tester who knows the app well but wasn't part of implementing this specific change — not a fully non-technical end user, and not a fellow developer reading code. Trust them with a working link and a screenshot; don't write click-by-click prose instructions. + +Match this real, established convention exactly: + +- Header: `### Manual testing`. +- Opening line, once, verbatim pattern: `Netlify: https://pr-.maps.netlify.dhis2.org/ + Instance: https://dev.im.dhis2.org/maps-app-42-3`. +- One bullet per Jira ticket, reusing the exact `[TICKET-ID](https://dhis2.atlassian.net/browse/TICKET-ID): ` link already used elsewhere in the PR body (under "Implements") — the ticket grouping _is_ the scenario grouping, not a separate numbered list. +- Under each ticket bullet, a nested "Test map(s)" list: `[ - main](#/)`, keyed by a real DHIS2 map/dashboard-item UID on the pinned Netlify preview. When the "Dashboard tested" checklist item applies, add a paired link with `?interpretationId=` labeled `- plugin` right next to the `- main` one. +- A screenshot per scenario block (`image`) as the visual "expected result" — never write textual "expected result:" prose instead. +- Use ` ` as a spacer line between ticket blocks (cosmetic, matches the real convention). +- No "Scenario 1 / Scenario 2" labeling, no numbered click-path steps. + +You'll be given the ticket list, the Netlify PR number(s)/instance URL, and real map/dashboard UIDs by the skill that invoked you — never invent a UID or fabricate a screenshot. If a UID or screenshot wasn't supplied for a ticket, leave that part as an explicit placeholder and say so, don't skip the bullet silently. diff --git a/.claude/commands/sonarqube-fix.md b/.claude/commands/sonarqube-fix.md new file mode 100644 index 0000000000..5b654b1e5d --- /dev/null +++ b/.claude/commands/sonarqube-fix.md @@ -0,0 +1,62 @@ +# SonarQube Issue Resolution Workflow + +Fix SonarQube quality gate issues for the current branch's PR, fetching issues directly from the SonarCloud API and addressing them in priority order. + +**Note**: this project (`dhis2_maps-app` on SonarCloud) is public, so every call below works anonymously — no `SONAR_TOKEN` needed. There is no local scanner/CI step to run — analysis happens automatically server-side on every push (SonarCloud "Automatic Analysis"), so there's no local "publish" step, just push and wait. + +## Instructions + +### 1. Identify the PR + +```bash +gh pr view --json number,title -q '"#\(.number) \(.title)"' +``` + +No `gh`/`GH_TOKEN` set up yet? Use the public GitHub REST API instead: + +```bash +git remote get-url origin # → parse org/repo +curl -s "https://api.github.com/repos///pulls?state=open&head=:$(git rev-parse --abbrev-ref HEAD)" \ + | jq -r '.[0].number' +``` + +### 2. Fetch and prioritize issues + +```bash +curl -s "https://sonarcloud.io/api/issues/search?componentKeys=dhis2_maps-app&pullRequest=&resolved=false&ps=100" \ + | jq -r '.issues[] | "\(.severity) - \(.type) - \(.message) - \(.component):\(.line)"' | sort +``` + +Fix order: **BLOCKER → CRITICAL → MAJOR → MINOR → INFO**, and within a severity, BUG before CODE_SMELL. Group by rule — fix every instance of the same rule together. + +Create a todo list (TodoWrite) with every issue before starting fixes, so progress stays visible. + +### 3. Fix in priority order + +For each issue: read the file for context, understand what's being flagged and why, apply the minimal fix that addresses it — don't refactor beyond what's reported. + +### 4. Test after each batch + +After every 3-5 related fixes: + +```bash +yarn lint && yarn test +``` + +Fix regressions immediately rather than accumulating unverified changes. + +### 5. Push and let Automatic Analysis catch up + +There's no local scanner to run. Push the commit, then re-poll the PR-scoped issues endpoint from step 2 — SonarCloud's GitHub App re-analyzes automatically on push, usually within a couple of minutes. Poll every 15-20s, capped at ~5 minutes so a stuck webhook doesn't hang the workflow. + +### 6. Done when + +- All todos completed +- `yarn lint && yarn test` pass +- The PR-scoped issues query returns none of the issues you fixed + +## Troubleshooting + +- **No matching PR** — confirm the branch has an open PR (`gh pr list` or the GitHub UI). +- **Issues still show after pushing** — re-analysis can lag a few minutes; re-poll rather than assuming the fix didn't take. If issues persist past ~5 minutes, check the PR's checks tab for a failed analysis run. +- **`gh` not authenticated** — not required; all the calls above work anonymously against this public repo. diff --git a/.claude/hooks/post-edit.sh b/.claude/hooks/post-edit.sh new file mode 100755 index 0000000000..554ba16d0d --- /dev/null +++ b/.claude/hooks/post-edit.sh @@ -0,0 +1,23 @@ +#!/usr/bin/env bash +set -uo pipefail + +input=$(cat) +file=$(printf '%s' "$input" | jq -r '.tool_input.file_path // empty') +[ -n "$file" ] && [ -f "$file" ] || exit 0 + +cd "${CLAUDE_PROJECT_DIR:-.}" || exit 0 + +case "$file" in + *.js | *.jsx | *.css | *.json | *.md | *.yml | *.yaml) ;; + *) exit 0 ;; +esac + +output=$(yarn d2-style apply "$file" 2>&1) +status=$? + +if [ "$status" -ne 0 ] || printf '%s' "$output" | grep -q '\[warn\]'; then + notes=$(printf '%s\n' "$output" | grep -vE '^\$ |^yarn run|^Done in|^info Visit|^\s*$') + jq -n --arg ctx "$notes" '{hookSpecificOutput: {hookEventName: "PostToolUse", additionalContext: $ctx}}' +fi + +exit 0 diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 0000000000..b737843e29 --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,27 @@ +{ + "permissions": { + "allow": [ + "mcp__grep__*", + "mcp__plugin_context7_*", + "mcp__plugin_chrome-devtools-mcp_*" + ] + }, + "hooks": { + "PostToolUse": [ + { + "matcher": "Edit|Write", + "hooks": [ + { + "type": "command", + "command": "bash .claude/hooks/post-edit.sh" + } + ] + } + ] + }, + "enabledPlugins": { + "chrome-devtools-mcp@claude-plugins-official": true, + "context7@claude-plugins-official": true + }, + "enabledMcpjsonServers": ["grep"] +} diff --git a/.claude/skills/branch-update/SKILL.md b/.claude/skills/branch-update/SKILL.md new file mode 100644 index 0000000000..dd7852ac8d --- /dev/null +++ b/.claude/skills/branch-update/SKILL.md @@ -0,0 +1,59 @@ +--- +name: branch-update +description: Bring the current feature branch up to date with master by merging (not rebasing) origin/master in — this team's real convention — then resolve any conflicts. Creates a merge commit, so only run when the user explicitly asks; notice and mention a stale branch, don't auto-run this. +disable-model-invocation: true +--- + +# Branch update + +This repo's real convention is **merge master into the feature branch**, repeatedly, during development — not rebase. The whole branch gets squash-merged into master via the PR at the end, so the intermediate merge commits never land on master's mainline; they only exist in the feature branch's own history. Don't rebase, and don't "clean up" these merge commits afterward — they're the norm, confirmed by real history (e.g. `Merge remote-tracking branch 'origin/master' into chore/setup-claude`). + +This skill creates a merge commit. `CLAUDE.md`'s "don't stage or commit unless explicitly asked" applies to merge commits too — that's why this skill requires explicit invocation. If you notice a branch is stale, say so; don't run this unasked. + +## Steps + +1. **Fetch.** + ``` + git fetch origin master + ``` +2. **Check how stale you actually are** before merging blindly: + ``` + git log --oneline HEAD..origin/master + git log --oneline origin/master..HEAD + ``` + Skip the merge entirely if there's nothing new to bring in. +3. **Merge.** + ``` + git merge origin/master + ``` +4. **No conflicts** — done. Mention the new merge commit exists; pushing it is a remote write and per this repo's universal rule needs its own explicit, in-the-moment ask, same as any other push. +5. **Conflicts** — see below. Never resolve by blindly taking "ours" or "theirs" wholesale; read both sides first. + +## Resolving conflicts + +For each conflicted file: + +1. Read the full conflict region plus enough surrounding context to understand _why_ each side changed it — not just what the diff lines say. `git log -p` on the conflicting commits from each side shows intent. +2. Prefer the minimal resolution that preserves _both_ changes' purpose — e.g. two additions to the same list/switch/reducer usually both belong, even when git can't auto-merge the surrounding lines. +3. After resolving, remove the conflict markers completely and re-read the result as if reviewing someone else's diff — a resolution that merges syntactically but silently drops one side's behavior is worse than an open conflict. +4. Run the touched files' tests (`npx jest `) plus `yarn d2-style check ` on every file you resolved, then the full `yarn lint && yarn test` before considering the merge done. + +### When to stop and ask instead of resolving + +Stop and hand back to the user — don't guess — when a conflict is a genuine business-logic collision, not line-adjacency noise: both sides changed the _behavior_ of the same function/condition in ways that don't obviously compose (one side changed a threshold, the other changed the formula it feeds into; one side removed a code path the other just extended). Signs it's this kind, not a mechanical one: + +- Resolving it requires deciding which behavior is "more correct," not just how to combine two edits. +- The two sides touch the same logic for unrelated reasons (different tickets), and combining them isn't obviously safe without domain knowledge you don't have. + +In that case, **leave the conflict markers in place** — don't `git merge --abort` unless the user asks you to. Run `git status` to show which files are still unresolved, describe what each side was trying to do, and ask the user how to reconcile them. Aborting loses the "you got this far" context for no benefit once you've already identified the ambiguity; leaving it in progress preserves both sides' intent for the user to inspect directly. + +## Done when + +- `git status` shows a clean merge (no unresolved paths). +- `yarn lint && yarn test` pass. +- The merge commit's message is left as git's default (`Merge branch 'master' into ` / `Merge remote-tracking branch 'origin/master' into `) — don't rewrite it to Conventional Commits format. These merge commits are exempt: they never land on master's mainline after the eventual squash-merge, and this team's real history confirms the default message is what's actually used. + +## Related + +- `commit-and-pr-messages` — for the _feature_ commits this branch carries, not the merge commit created here. +- `pr-chain` — has its own, more involved version of this problem once a stack member actually squash-merges (a plain merge stops working at that point; see that skill's `references/squash-merge-sync.md`). diff --git a/.claude/skills/claude-stack-retro/SKILL.md b/.claude/skills/claude-stack-retro/SKILL.md new file mode 100644 index 0000000000..efb63b19a8 --- /dev/null +++ b/.claude/skills/claude-stack-retro/SKILL.md @@ -0,0 +1,60 @@ +--- +name: claude-stack-retro +description: Use at a natural checkpoint after a chunk of work — end of a session, right after a PR merges, weekly — to look back over that stretch (which may span many separate Claude Code conversations over days or weeks) and propose concrete updates to this repo's own .claude/ tooling: new/changed skills, missing subagents, CLAUDE.md gaps. Never edits .claude/skills/*, .claude/agents/*, .claude/commands/*, or CLAUDE.md itself — proposes only, and logs the outcome so a rejected idea doesn't resurface. +disable-model-invocation: true +--- + +# Claude stack retro + +Explicit `/claude-stack-retro` invocation only. Its own side effects are light (it writes a local log + a local state file, nothing else) — it's gated on explicit invocation because _when_ to retrospect is a deliberate checkpoint the user picks, not something worth guessing at automatically. + +| Situation | Do this | +| ------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------ | +| First run in this repo (`.claude/retros/.state.json` doesn't exist yet) | Bootstrap the window — see `references/evidence-gathering.md` | +| Evidence from steps 2-6 doesn't add up to anything concrete | Ask the user directly what was frustrating/manual/surprising — a "nothing" answer is a complete, valid retro | +| A candidate finding's fingerprint matches `rejectedFingerprints` in state | Skip it silently — don't re-propose (see `references/output-and-state.md`) | +| A candidate touches `.claude/skills/*`, `.claude/agents/*`, `.claude/commands/*`, `CLAUDE.md`, or `.claude/settings.json` | Never edit directly, ever — propose only | +| Checking whether a PR/merge actually happened as evidence | Read-only `gh`/`curl` only — never `gh pr merge/edit/ready`, per this repo's universal remote-write rule | + +## What this is (and isn't) + +This is about **this repo's own `.claude/` tooling** — skills, subagents, `CLAUDE.md`, hooks, commands. It is a different, narrower thing than the assistant's cross-project memory system (`~/.claude/projects/.../memory/`) — memory persists preferences/corrections _across_ repos and sessions; this skill's output is proposed edits to files committed _in this one repo_. It reads memory as one input signal (an unresolved `feedback-*` entry may point at a real gap in a skill) but never writes memory entries itself — that stays the memory system's own job. + +## 1. Determine the review window + +Read `.claude/retros/.state.json`. If present, the window is `lastReviewedSha..HEAD`. If missing, this is the first run — see "Bootstrapping the window" in `references/evidence-gathering.md` rather than guessing at a start point. + +## 2. Gather evidence + +Full detail and exact commands in `references/evidence-gathering.md`. In priority order: git log/diff since the marker (with `.claude/**` changes called out separately) → this thread's own context, free, if you're at the tail end of the session that did the work → memory index entries dated inside the window → `.claude/settings.local.json`'s allow-list as a careful signal (see the caution there — never propose committing that file itself) → other sessions' transcripts since the marker, best-effort/secondary → asking the user directly if the above is thin. + +## 3. Analyze — don't manufacture findings + +Work through `references/analysis-checklist.md`. The governing rule, adapted from Anthropic's own reviewer-gap caution ("a reviewer prompted to find gaps will usually report some, even when the work is sound"): **only surface a suggestion that would plausibly recur, or that already caused friction or a mistake this stretch** — never "this could theoretically be tidier." If nothing clears that bar, say so and stop; a short or empty retro is a correct outcome. + +## 4. Fresh-eyes pass (only if step 3 produced at least one candidate) + +Dispatch a plain, fresh-context subagent (Task tool, general-purpose — no bespoke persona file needed, same mechanism `implement-plan` uses for its own end-of-run diff review, and the same "use an Explore subagent" pattern already established in `dhis2-web-api-research`) with **only the raw evidence** you gathered in step 2 — git log/diff excerpts, transcript excerpts, memory entries — not your own draft candidate list. State explicitly in its prompt: read-only, no Edit/Write/git-write/gh-write, just analyze the evidence against the same checklist and report independently. + +Reconcile: keep anything either of you found with solid evidence behind it. Drop anything only you found that the fresh pass didn't independently surface and that isn't obviously supported by the evidence alone — that combination is a sign of rationalizing rather than observing. + +## 5. Present, never auto-apply + +For each surviving suggestion, in this shape (full template in `references/output-and-state.md`): **what** (concrete file/skill/section) / **why** / **evidence** / a **sketch** of the change (a short diff-style snippet for a small edit, or a frontmatter + section-header outline for a new skill/subagent — never a fully polished file inline) / **how to apply** (re-enter Plan Mode for anything nontrivial; direct in-the-moment approval for a one-line addition). + +Ask per suggestion, not once for the whole batch — accept / reject / defer. Nothing under `.claude/skills/*`, `.claude/agents/*`, `.claude/commands/*`, `CLAUDE.md`, or `.claude/settings.json` is touched until the user says so for that specific item. + +## 6. Log and update state + +Write `.claude/retros/YYYY-MM-DD.md` recording every suggestion and its outcome, and update `.claude/retros/.state.json` (`lastReviewedSha`/`lastReviewedDate`, plus any newly-rejected fingerprint). Exact schemas in `references/output-and-state.md`. Writing these files is a local edit like any other (no ask needed — same as `spec-from-ticket` writing its spec file) — but `git add`/`git commit` on them still needs the user's explicit ask, per this repo's existing convention. + +## Scope discipline + +- **Cap ~5-7 suggestions per run.** More than that means the window is too large — say so, and recommend running retros more often, rather than dumping a long backlog in one pass. +- **Skip one-offs.** A single typo or a one-time workaround for something already fixed doesn't earn a slot — only recurring/structural friction does. +- **Prefer the smallest fix.** A one-line addition to an existing skill's decision table or to `CLAUDE.md` beats a brand-new skill file whenever it covers the same ground — new skill/subagent files are the heaviest suggestion type, reserve them for genuinely repeated multi-step workflows (mirrors `pr-chain`'s own "when to stop and re-plan" restraint). +- **Never** edits `.claude/skills/*`, `.claude/agents/*`, `.claude/commands/*`, `CLAUDE.md`, or `.claude/settings.json` itself, and never runs `git push`/`gh pr *` — every suggestion is proposed, the user decides, every time, per this repo's universal remote-write/no-silent-tooling-edit rule. + +## Optional argument + +`/claude-stack-retro --since ` overrides the stored marker for this run's _analysis_ start point without corrupting state — `.state.json` still advances to the real `HEAD` at the end of the run, same as a normal invocation. diff --git a/.claude/skills/claude-stack-retro/references/analysis-checklist.md b/.claude/skills/claude-stack-retro/references/analysis-checklist.md new file mode 100644 index 0000000000..cd3f81cb0a --- /dev/null +++ b/.claude/skills/claude-stack-retro/references/analysis-checklist.md @@ -0,0 +1,14 @@ +# Analysis checklist + +Work through these; a "no" is a fine answer for most of them, most of the time. The bar for surfacing a suggestion: **it would plausibly recur, or it already caused friction or a mistake this stretch** — not "this could theoretically be tidier." (The same caution Anthropic's own best-practices doc gives about review prompts always finding _something_ — don't manufacture busywork here either.) + +1. **Repeated manual sequence → candidate skill.** The same ≥3-step Bash/tool sequence shows up across ≥2 separate sessions or commits in the window, with no skill covering it yet. +2. **Existing skill gave wrong/stale/incomplete guidance.** A skill or reference doc names a command, file, or convention that a commit in the window changed or removed; or the transcript shows the user correcting something a skill said. +3. **New file/architecture pattern `CLAUDE.md` doesn't mention.** A new top-level directory, state pattern, build step, or repo convention appears in the window's commits with no matching line in `CLAUDE.md`. +4. **A subagent would have helped but didn't exist.** The transcript/history shows the same skill improvising a different tone/persona more than once — not "this would be nice," only "this concretely happened and cost something." +5. **A mid-session correction reveals a missing standing rule.** The user said "don't do X" / "actually we always Y" — check it isn't already captured in `CLAUDE.md`, a skill, or memory; if genuinely new and phrased like a standing rule (not a one-off aside), it's a candidate `CLAUDE.md`/skill line. (This is exactly how this repo's own universal remote-write rule was born — one clear correction, documented once, applied everywhere. One clear correction can be enough; ten vague mentions of the same thing are one finding, not ten.) +6. **A referenced command/file no longer resolves.** Spot-check commands named in touched skills against `package.json` scripts / actual file paths — a skill telling the agent to run something that no longer exists is always worth flagging, regardless of how it was found. +7. **Old-style artifact newly inconsistent.** e.g. `.claude/commands/sonarqube-fix.md` predates the skills convention — true every retro, so don't flag it every retro just because it's still true; only surface it if something in _this_ window makes the inconsistency newly load-bearing (e.g. it needed the universal remote-write rule applied to it and doesn't have it). +8. **`.claude/settings.local.json` drift** — see the caution in `references/evidence-gathering.md`. + +Do not turn this into a linter. If nothing here clears the "would recur / already cost something" bar, the correct output is "nothing rose to that bar this stretch" — a complete, successful run, not a weak one. diff --git a/.claude/skills/claude-stack-retro/references/evidence-gathering.md b/.claude/skills/claude-stack-retro/references/evidence-gathering.md new file mode 100644 index 0000000000..b45198c5eb --- /dev/null +++ b/.claude/skills/claude-stack-retro/references/evidence-gathering.md @@ -0,0 +1,46 @@ +# Evidence gathering + +## 1. Review window + +Read `.claude/retros/.state.json`. If present, the window is `lastReviewedSha..HEAD`. + +**Bootstrapping (first run, no state file yet):** don't default to this repo's entire history — that's almost certainly too large a window to produce a useful, bounded retro. Ask the user: "No prior retro state — since when should I look? Since the `.claude/` tooling was first added (`git log --diff-filter=A --format=%H -- CLAUDE.md | tail -1`), or a shorter window (last 30 days / since the last release tag / a specific date)?" State whichever is chosen in the eventual retro log. + +## 2. Git history in the window + +```bash +git log --oneline # overview +git log --stat -- .claude/ # did the tooling change by hand, + # outside this skill? worth understanding + # why, and noting even without a "suggestion" +git log --name-only # spot new top-level dirs/patterns +git diff -- package.json # new scripts/deps implying new + # commands worth documenting +``` + +## 3. This thread's own context + +Free — no tool call needed. If this retro runs at the tail end of the session that did the work, everything discussed is already here: corrections the user gave, commands that failed and got worked around, places you had to ask the user something a skill should already have told you. + +## 4. Other sessions' transcripts since the window started (best-effort, secondary) + +Claude Code stores one JSONL transcript per session, under a per-project directory whose name is this repo's absolute path with `/`, `_`, and `.` replaced by `-`: + +```bash +slug=$(printf '%s' "$CLAUDE_PROJECT_DIR" | sed 's/[\/_.]/-/g') +ls -t ~/.claude/projects/"$slug"/*.jsonl +``` + +Skim (grep/Read, don't fully parse the JSONL structure) files modified since `lastReviewedDate` for: the user correcting a factual/procedural claim, a tool call repeated many times in a row (thrashing), an explicit "that's frustrating" / "why doesn't X exist" remark. Treat this as **color, not primary evidence** — this is undocumented internal storage and its format could change between Claude Code versions; if the directory or files aren't there, skip silently and lean on git history + the interactive question instead. + +## 5. Memory index + +Same project slug, under `memory/` instead of the transcript directory: `~/.claude/projects//memory/MEMORY.md` plus any `feedback-*.md`/`project-*.md` entries dated inside the window. A `feedback-*` entry is a strong signal — it exists because something already surprised or corrected the assistant once. If it also would have changed a skill's or `CLAUDE.md`'s guidance, that's this skill's business — cite it as evidence, but never edit or create memory files here. + +## 6. `.claude/settings.local.json` signal (careful) + +Read its `permissions.allow` list. A repeated, clearly-safe, repo-wide entry (e.g. a read-only `curl`/`git log` pattern used across multiple sessions) is worth a "promote to committed `settings.json`" suggestion. Do **not** propose promoting anything that looks personal or machine-specific (local absolute paths, one-off debug commands) or anything write-capable — err toward not suggesting if unsure. This file is gitignored on purpose (personal, per this repo's convention); most of its entries should stay personal. + +## 7. Ask, if evidence is thin + +If steps 2-6 don't add up to anything concrete, ask directly: "What was frustrating, manual, or surprising this stretch?" — one open question, not a checklist interrogation. "Nothing, it was smooth" is a valid, complete answer. diff --git a/.claude/skills/claude-stack-retro/references/output-and-state.md b/.claude/skills/claude-stack-retro/references/output-and-state.md new file mode 100644 index 0000000000..5891395e87 --- /dev/null +++ b/.claude/skills/claude-stack-retro/references/output-and-state.md @@ -0,0 +1,64 @@ +# Output format and state + +## Presenting suggestions + +One run produces 0-7 suggestions (see Scope discipline in `SKILL.md`). Present each as: + +### N. \ + +- **What**: one line, concrete — name the file/skill/`CLAUDE.md` section. +- **Why**: the friction/mistake it's tied to. +- **Evidence**: e.g. "seen in 3 sessions this window: \" or "user correction on 2026-07-15: '\'" or "`` still tells the agent to run ``, removed in ``". +- **Proposed change**: a short diff-style snippet (for a `CLAUDE.md` line or an existing skill's decision-table row), or a frontmatter + section-header outline (for a new skill/subagent) — never a fully polished file inline; that's what the follow-up Plan Mode pass is for. +- **How to apply**: "re-enter Plan Mode on this one" for anything nontrivial (new skill/subagent, `CLAUDE.md` restructuring), or "say the word and I'll make this small edit directly" for a one-line addition. + +Ask per suggestion, not once for the whole batch — accept / reject / defer. Nothing under `.claude/skills/*`, `.claude/agents/*`, `.claude/commands/*`, `CLAUDE.md`, or `.claude/settings.json` gets touched until the user says so for that specific item. + +## Retro log + +Write `.claude/retros/YYYY-MM-DD.md` (append `-2`, `-3` if a second/third retro lands the same day): + +```markdown +# Retro — YYYY-MM-DD + +Window: .. () + +## Suggestions + +### 1. — accepted + +<what/why/evidence, 2-4 lines> +Applied as: <what actually got written, or "deferred to a follow-up session"> + +### 2. <title> — rejected + +<what/why/evidence> +Reason given: "<user's stated reason, verbatim or close to it>" + +### 3. <title> — deferred + +<what/why/evidence> +``` + +Keep it short — a decision log, not a transcript dump. + +## State file + +`.claude/retros/.state.json`: + +```json +{ + "lastReviewedSha": "<full 40-char sha of HEAD at the time this retro ran>", + "lastReviewedDate": "YYYY-MM-DD", + "rejectedFingerprints": [ + "cypress-flake-triage-skill", + "promote-yarn-build-verbose-flag" + ] +} +``` + +`rejectedFingerprints`: a kebab-case slug of each rejected suggestion's title (lowercase, non-alphanumeric → `-`, collapse repeats). Before including a candidate in a future run, slug its title the same way and skip it silently on a match — a plain "already said no to this" check, not fuzzy matching. If the same underlying friction resurfaces with clearly new evidence (e.g. a third occurrence after being dismissed as a one-off), it's fine to re-raise it — say explicitly that it was rejected before and why this time is different, rather than silently re-proposing. + +Deferred suggestions are **not** fingerprinted — they weren't rejected, just not acted on yet, so a future retro can re-surface a still-relevant deferred item. + +Update this file every run, even a "nothing to report" one — it still moves `lastReviewedSha`/`lastReviewedDate` forward so the next run's window isn't re-scanned from scratch. diff --git a/.claude/skills/commit-and-pr-messages/SKILL.md b/.claude/skills/commit-and-pr-messages/SKILL.md new file mode 100644 index 0000000000..f656b1807f --- /dev/null +++ b/.claude/skills/commit-and-pr-messages/SKILL.md @@ -0,0 +1,111 @@ +--- +name: commit-and-pr-messages +description: Use before drafting a commit message, a PR title, or a PR description body — covers this repo's Conventional Commits format, the Jira ticket bracket convention, the real PR template, and the check-tasklist.yml constraint that blocks a PR on any unchecked checkbox. +--- + +# Commit and PR messages + +A lookup, not a workflow — consult the row that matches what you're drafting, then apply the rules below it. + +| Drafting... | What's different | +| --------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| A commit message | Header must be a valid Conventional Commit; never hand-append `(#NNNN)`; never hand-write a `chore(release):` message | +| A PR title | Same rules as a commit message — this repo squash-merges, so the PR title _becomes_ the final commit header. maps-app's CI doesn't lint-check this today (event-visualizer-app's does, via `wagoid/commitlint-github-action`), but write it as if it will be checked | +| A PR description body | Start from `.github/pull_request_template.md` verbatim; every `- [ ]` in the final body must be either genuinely checked or replaced with `_N/A_` — see "Checklist constraint" below | + +## Conventional Commits format + +`<type>(<scope>)?: <subject>` — enforced locally by `.hooks/commit-msg` (`yarn d2-style check commit "$1"`, i.e. commitlint via `@dhis2/cli-style`'s `@commitlint/config-conventional`). **Not** re-verified in maps-app's own CI (unlike event-visualizer-app), so the local hook is the only real gate — don't rely on CI to catch a bad message. + +- **Allowed types**: `build`, `chore`, `ci`, `docs`, `feat`, `fix`, `perf`, `refactor`, `revert`, `style`, `test` — but real usage on `master` is dominated by just three: `feat`, `fix`, and plain `chore` (+ scoped variants `chore(deps)`, `chore(deps-dev)`, `chore(release)`, `chore(app-platform)`, `fix(translations)`). Don't reach for an exotic type just because it's in the allowed list if `feat`/`fix`/`chore` already fits. +- Header ≤ 120 chars. +- Subject: not sentence-case, Start Case, PascalCase, or UPPERCASE; no trailing period. +- Scope is optional and rare in this repo's history — don't invent one just to fill the slot. + +## Bot-generated patterns — recognize these, never hand-write them + +- `chore(release): cut X.Y.Z [skip release]` — semantic-release only. +- `chore(deps): bump <pkg> from A to B (#NNNN)` / `chore(deps-dev): bump ...` / `chore(deps): bump the dependencies group across 1 directory with N updates` — Dependabot only. +- `fix(translations): sync translations from transifex (master)` — Transifex sync bot only. + +## Jira ticket references + +`[PROJECTKEY-NUMBER]` in square brackets — the project key isn't always `DHIS2` (`CLIM-501` also appears in history). Placed after the description, before any `(#PRNUM)`. **Multiple tickets in one bracket-stack is normal and common** for a commit/PR that batches related fixes: + +``` +fix: prevent timeline crash on load failure and fix period after drilling up/down [DHIS2-19063] [DHIS2-21113] (#3664) +fix: various bubble layer issues [DHIS2-15696] [DHIS2-19209] [DHIS2-19447] [DHIS2-19448] (#3550) +``` + +## What makes a real "good" message here + +The healthiest real examples describe the **observable behavior/outcome** — what a user or future maintainer would see changed — not the implementation mechanics: + +``` +fix: prevent duplicate overview map outline in splitmap download mode [DHIS2-21540] (#3676) +fix: preserve program/enrollment period type on TE layer reload [DHIS2-19205] (#3674) +feat: add custom scale toggle for heat stress layers [DHIS2-20564] (#3708) +fix: resize data table and map canvas continuously during drag [DHIS2-15884] (#3675) +``` + +Not "refactor the useEffect to guard against null" — "prevent X crash," "preserve Y on Z." Most real headers run ~40-70 chars, well under the 120 hard cap. Lowercase immediately after the type/colon, no trailing period, imperative/active verb. + +## Two things to never fabricate + +- **`(#PRNUM)`** — GitHub appends this automatically when the PR is squash-merged. Never write it into a drafted commit message or PR title yourself; you don't know what the number will be, and a wrong one is worse than none. +- **`chore(release): cut X.Y.Z [skip release]`** — generated by semantic-release on release. Never hand-write one. + +## PR description body + +Use `.github/pull_request_template.md` as the literal skeleton — don't paraphrase its section headers or reorder them: + +```markdown +Implements [DHIS2-XXXX](https://dhis2.atlassian.net/browse/DHIS2-XXXX) + +### Description + +_text_ + +--- + +### Quality checklist + +Add _N/A_ to items that are not applicable. + +- [ ] Dashboard tested +- [ ] Cypress and/or Jest tests added/updated +- [ ] Docs added +- [ ] d2-ci dependencies replaced (analytics or maps-gl link https://github.com/dhis2/[lib]/pull/XXX) +- [ ] Tester approved (name) + +--- + +### ToDos + +- [ ] _todo_ + +--- + +### Known issues + +- [ ] _issue_ + +--- + +### Screenshots + +_supporting images_ +``` + +### Checklist constraint (`check-tasklist.yml`) + +`Shopify/task-list-checker` scans the whole PR body on every edit and blocks the PR while _any_ `- [ ]` remains, anywhere in the body — not just the Quality checklist. When drafting or updating a description: + +- Only leave `- [ ]` on items you genuinely expect to complete before merge. +- For anything not applicable, write `_N/A_` in place of the checkbox — that's the template's own escape hatch, not a workaround. +- It's fine to leave items unchecked while the PR is still a draft; just don't hand it off as "ready for review" with stray open boxes. + +## Related + +- `pr-polish` — the workflow that keeps the Quality checklist honest across rounds of fixes; this skill just supplies the format rules it applies. +- `pre-review` — final check before flipping a PR to ready; doesn't touch message drafting, but its title/description check reuses this skill's template. diff --git a/.claude/skills/community-post/SKILL.md b/.claude/skills/community-post/SKILL.md new file mode 100644 index 0000000000..91036933d7 --- /dev/null +++ b/.claude/skills/community-post/SKILL.md @@ -0,0 +1,28 @@ +--- +name: community-post +description: Use when drafting a DHIS2 Community of Practice release-announcement post for a new maps-app feature/fix — gathers version/tickets/tone/prerequisites from the user, then dispatches the community-post-writer subagent to draft the post matching the real house format. +--- + +# Community post + +Drafts a release-announcement post for community.dhis2.org, matching a real, consistent format verified across multiple published posts. + +## Gather inputs + +Before dispatching, collect from the user (suggest, don't assume, where you can): + +- **App version(s)** — can suggest from `package.json`, but confirm since Play/App Hub publishing timing may lag the repo's own version. +- **The feature(s)/fix(es) to cover** — Jira ticket IDs. Jira is the house link convention; never link a GitHub PR in the post itself. +- **Audience/tone lever** — default: accessible to non-developer admins/analysts, matching all real examples. Flag if this one should be more technical (rare). +- **Feature-specific prerequisites** — e.g. "requires a Google Earth Engine API key configured," "requires organisation unit polygons." This is domain knowledge the skill can't guess; ask, or pull from the relevant ticket/PR description if available. +- **Urgent patch note?** — only needed if this is a bugfix release superseding a broken version (e.g. "update directly to 100.8.1"). +- **Screenshots** — the user supplies real images or says "I'll add them later." Never fabricate a fake UI image; no GIFs in any real example. +- **Who's signing** — name + title (PM and developer voices are both real precedents). + +## Dispatch + +Hand all of the above to the **`community-post-writer`** subagent to draft the actual post. Review the draft against the real structure before presenting it: opening line, per-app compatibility sentence, one heading per feature with a screenshot placeholder and a "Jira"-anchor-texted link, closing, signature — no fabricated call-to-action, no fabricated screenshot description. + +## Note + +Posting to the forum is manual, by the user — this skill only drafts the text. diff --git a/.claude/skills/dhis2-web-api-research/SKILL.md b/.claude/skills/dhis2-web-api-research/SKILL.md new file mode 100644 index 0000000000..4b6b634fad --- /dev/null +++ b/.claude/skills/dhis2-web-api-research/SKILL.md @@ -0,0 +1,38 @@ +--- +name: dhis2-web-api-research +description: Use when writing or modifying code that calls the DHIS2 Web API (analytics, geoFeatures, tracker, metadata endpoints) and the exact request/response shape is unclear. Prevents guessing/hallucinating API fields. +--- + +# DHIS2 Web API research + +Don't guess field names or response shapes from memory — DHIS2's API surface is large and versioned. Work through these tiers, cheapest first, stopping as soon as you have enough certainty. + +| Scenario | Do this | +| --------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Need the shape of a specific endpoint/resource | Tier 1: fetch the scoped OpenAPI spec | +| Need to see real data (pagination, actual field values, edge cases) | Tier 2: probe a live instance | +| OpenAPI spec is ambiguous or you need to understand server-side logic | Tier 3: read the DHIS2 backend source | +| Just need to know which endpoints this app already calls | See `d2.config.js`'s `omitPatterns` list at repo root — it's a ready-made inventory: `geoFeatures`, `analytics`, `tracker/trackedEntities`, `organisationUnitGroupSets`, `dataElements`, `trackedEntityAttributes`, `optionSets`, `legendSets`, `programs`, `programStages`, `trackedEntityTypes`, `relationshipTypes`, `organisationUnitLevels`. | + +## Tier 1 — OpenAPI spec (cheapest) + +Fetch the spec scoped to the resource you care about via `curl`, not the whole multi-MB spec. Read only the relevant schema/paths. + +## Tier 2 — Probe a live instance + +GET-only, against a dev/test instance only (never write). Paginate — don't pull whole collections. Good for confirming actual field values and edge cases the spec doesn't show. + +## Tier 3 — Read the backend source + +When the spec is ambiguous or you need server-side validation/business logic: + +``` +npx opensrc dhis2/dhis2-core --modify +``` + +This clones the DHIS2 backend Java source into a gitignored `./opensrc/` directory. The codebase is large — use an Explore subagent to search it rather than reading broadly yourself. + +## Troubleshooting + +- **`opensrc` clone already exists and looks stale** — re-run with `--modify` to refresh, or check the pinned DHIS2 version this app targets (`minDHIS2Version` in `d2.config.js`) against what you cloned. +- **Live probe returns 401/403** — you're likely pointed at a non-dev instance or missing `cypress.env.json` credentials; don't escalate to write attempts to work around this. diff --git a/.claude/skills/docs-update/SKILL.md b/.claude/skills/docs-update/SKILL.md new file mode 100644 index 0000000000..e52e33a248 --- /dev/null +++ b/.claude/skills/docs-update/SKILL.md @@ -0,0 +1,43 @@ +--- +name: docs-update +description: Use when a code change is user-facing and maps-app's published documentation (docs/src/*.md -> docs/maps.md) may need updating — maps the feature area to the right docs/src file and dispatches the docs-writer subagent to draft the prose and rebuild. +--- + +# Docs update + +Keeps `docs/src/*.md` → `docs/maps.md` in sync with a user-facing code change. Advisory, not a gate — no CI enforces this, and not every change needs it (historically only ~24% of `feat`/`fix` commits touch `docs/` at all). Use judgment about whether a given change is even docs-relevant. + +## Feature-area → file mapping + +| Feature area | `docs/src/` file | +| ---------------------------------- | ------------------------------------- | +| Thematic layers | `04-thematic-layer.md` | +| Event layers | `05-event-layer.md` | +| Tracked entity layers | `06-tracked-entity-layer.md` | +| Facility layers | `07-facility-layer.md` | +| Org unit layers | `08-org-unit-layer.md` | +| Earth Engine layers | `09-earth-engine-layer.md` | +| External map layers | `10-external-map-layers.md` | +| Org unit profile | `11-org-unit-profile.md` | +| File menu | `12-file-menu.md` | +| Interpretations | `13-interpretations.md` | +| Image export | `14-image-export.md` | +| Search | `15-search.md` | +| Measure distance | `16-measure-distance.md` | +| Admin-only features | `18-administrator.md` | +| General create-map flow / basemaps | `02-create-map.md` / `03-basemaps.md` | + +## Flow + +1. Identify the affected file(s) from the table above. +2. Dispatch the **`docs-writer`** subagent with the feature description and affected file(s) — its job is drafting the prose (matching the existing `> **Note**` callout and heading-ID conventions) and running `yarn docs:build` to regenerate `docs/maps.md`. +3. Review the diff (both the `docs/src/*.md` edit and the regenerated `docs/maps.md`) before it's committed — still gated by `CLAUDE.md`'s "don't stage or commit unless explicitly asked." +4. Commit the source edit and the regenerated `docs/maps.md` together, in the same commit/PR as the feature — this is the dominant real historical pattern, not a separate docs follow-up. + +## Screenshots + +If the change alters something shown in an existing screenshot, flag it ("this may need recapturing") rather than fabricating a new image — real docs screenshots are actual app captures, never synthesized. + +## Related + +- `commit-and-pr-messages` — the "Docs added" Quality-checklist item this skill helps make honestly checkable. diff --git a/.claude/skills/implement-plan/SKILL.md b/.claude/skills/implement-plan/SKILL.md new file mode 100644 index 0000000000..0f121568a9 --- /dev/null +++ b/.claude/skills/implement-plan/SKILL.md @@ -0,0 +1,62 @@ +--- +name: implement-plan +description: Autonomously executes a written implementation plan (from spec-from-ticket, or pasted/referenced directly) through a commit-by-commit implement/test/self-review/commit cycle, requesting permissions up front rather than piecemeal. Explicit invocation only — run with /implement-plan <path-to-spec-or-plan>. +disable-model-invocation: true +--- + +# implement-plan + +Executes a plan (a `.claude/specs/*.md` file, or one pasted/described directly) through a disciplined commit-by-commit cycle, with **local** permissions negotiated once up front — because nobody is available to approve tool calls piecemeal for the duration of the run. Remote writes (push, PR create/edit/ready) are never part of that negotiation — see "Handoff." + +| Situation | Do this | +| ------------------------------------------------------------------------- | -------------------------------------------------------------------- | +| Starting a fresh run | Do the permission negotiation (step 1) before touching anything else | +| A plan step is ambiguous, or the code doesn't match what the plan assumed | Stop, ask — see "Escalation conditions" | +| A step is done, tests pass, self-review is clean | Commit it, move to the next step | +| Every step is done and committed | Full suite, then a fresh-context full-diff review — see step 5 | +| Ready to push / open a PR | Stop and ask explicitly, every time — see "Handoff" | + +## 1. Negotiate permissions — before anything else + +Enumerate exactly what this run will need and get it granted once, up front, rather than mid-flight. Full script and the concrete Bash allowlist: `references/permission-negotiation.md`. In short, ask the user to either: + +- grant a session allowlist covering lint/test/build and **local** git (add/commit/status/diff/log/merge), or +- switch to Claude Code's auto mode (`/permission-mode auto`) for the run. + +**Never include `git push`, `gh pr create/edit/ready`, or any other remote-write command in this negotiation** — no matter how far ahead the user wants to go. This repo's universal rule (see `CLAUDE.md`) is that remote writes are never pre-negotiated or bundled into a blanket allowlist; they're handled fresh, per the "Handoff" section below, only when actually reached and only with a fresh explicit ask at that moment. + +## 2. Break the plan into commits + +If the plan document already has numbered, file-scoped "Implementation plan" steps (the `spec-from-ticket` shape), treat each as one commit unless it's too large (spans unrelated files — split it) or too small/coupled to compile independently (merge it with its neighbor). If working from a looser/pasted plan, derive equivalent units yourself. Full heuristic and commit-message drafting rules (Conventional Commits + the real `[TICKET-ID]` bracket convention — see the `commit-and-pr-messages` skill for the format itself, **never** fabricating the `(#PRNUM)` suffix GitHub appends at squash-merge): `references/commit-cycle.md`. + +Track progress with `TodoWrite` as you go — this is a multi-step, potentially long-running loop. + +## 3. The cycle, per step + +1. Implement the step. +2. Test/lint the touched files only — reuse `CLAUDE.md`'s "Lint/test workflow for agents" verbatim (`npx jest <file>`, then `yarn d2-style check <file>`), don't reinvent it. +3. Self-review just this step's diff — invoke `/code-review` at low/medium effort. Address findings; don't blindly `--fix` — surface what was found so there's an audit trail. +4. Commit (message per `commit-and-pr-messages`). +5. Next step. + +## 4. Escalation conditions — stop and ask, don't guess + +- The plan's assumption about the code no longer holds (file renamed/moved, prop already exists, etc.). +- A test fails and the fix requires a judgment call about behavior, not a mechanical fix. +- A step would touch something the plan's "Out of scope" section excludes, or wasn't anticipated at all. +- A step needs a cross-repo change (e.g. `@dhis2/maps-gl`) the plan didn't flag as needing coordination. +- A pre-existing, unrelated test/lint failure shows up (don't silently "fix" things outside the plan's scope — flag it and ask whether to proceed past it or address it first). +- Anything needs a tool/command outside what was negotiated in step 1 — pause and ask for that specific addition rather than expanding scope silently. + +## 5. After the last step + +1. Full suite: `yarn lint && yarn test` (and `yarn cy:run --spec ...` if the plan's verification section calls for it). +2. Fresh-context full-diff review: dispatch a subagent to run `/code-review` (or read the diff cold) across the _whole_ diff, not just the last step — the main session's context is full of the implementation by now, a fresh pair of eyes catches what a tired context misses. +3. Address findings as additional small commits (never amend prior commits — new commits only, per this repo's git norms), unless the user explicitly asks to clean up history before it's pushed. +4. Report a summary: commits made (SHAs + messages), test/lint/e2e status, current branch. If mid-flight master moved significantly, this is also where you'd have merged master into the feature branch (merge, not rebase — this team's convention, see `branch-update`) and re-run the suite; mention if that happened. + +## Handoff + +Stop here by default and ask before pushing/opening a PR — always, no exceptions, regardless of anything negotiated in step 1. Explicit `/implement-plan` invocation is permission to commit locally throughout (per `CLAUDE.md`'s "don't stage or commit unless explicitly asked," which this explicit invocation satisfies) — it is **not** permission for anything that writes to a remote. Push and PR-create are qualitatively different, harder-to-reverse actions, and this repo's universal rule requires a fresh, explicit, in-the-moment ask for those specifically, every single time, with no pre-provisioned credential or bundled approval to route around that ask. + +If the user explicitly says to proceed: push, then `gh pr create` filling the real template (`.github/pull_request_template.md`) — `Implements [TICKET-ID](url)`, Description, Quality checklist (check what's actually true, `_N/A_` the rest — `check-tasklist.yml` blocks the PR on **any** unchecked `- [ ]` anywhere in the body, not just that section), ToDos, Known issues, Screenshots. Then hand off to the `pre-review` skill for the final self-review-and-mark-ready pass — don't duplicate its logic. diff --git a/.claude/skills/implement-plan/references/commit-cycle.md b/.claude/skills/implement-plan/references/commit-cycle.md new file mode 100644 index 0000000000..2686593580 --- /dev/null +++ b/.claude/skills/implement-plan/references/commit-cycle.md @@ -0,0 +1,18 @@ +# Commit granularity and message drafting + +## Splitting/merging plan steps into commits + +- Default: one commit per numbered "Implementation plan" step, if the plan came from `spec-from-ticket` (that skill deliberately writes steps at commit granularity). +- Split a step if it spans clearly unrelated files/concerns. +- Merge adjacent steps if either one alone would leave the tree non-compiling or tests red (e.g. "add action creator" + "wire into reducer" — neither makes sense alone). +- Rule of thumb before committing: `yarn lint` and the touched-file tests should be green. Avoid deliberately-broken intermediate commits. + +## Commit message format + +See the `commit-and-pr-messages` skill for the full format rules and real examples. In short: Conventional Commits, `<type>: <description> [<TICKET-ID>]`, header ≤120 chars, **never** append `(#PRNUM)` (GitHub adds that automatically at squash-merge), **never** hand-write a `chore(release):` message. + +These are step-commits on a feature branch that will eventually be **squash-merged** — so the final PR title matters more for permanent history than any individual step message, but step messages still matter for reviewers reading commit-by-commit, so don't get sloppy just because they'll be squashed away. + +## Mid-flight master sync + +If the run is long enough that `master` moves meaningfully, **merge** master into the feature branch (this team's real convention — see the `branch-update` skill — never rebase), then re-run the full suite before continuing. This falls under the pre-negotiated `git merge*` allowlist entry — do it, but say so in the summary, don't do it silently. diff --git a/.claude/skills/implement-plan/references/permission-negotiation.md b/.claude/skills/implement-plan/references/permission-negotiation.md new file mode 100644 index 0000000000..f11b9bfeb7 --- /dev/null +++ b/.claude/skills/implement-plan/references/permission-negotiation.md @@ -0,0 +1,28 @@ +# Permission negotiation script + +Run this before touching any file. Use `AskUserQuestion`. + +## The one question — local operations only + +> This will run for a while without stopping to ask each time. Pick one: +> (a) Grant the allowlist below for this session (I'll ask once via the permission prompt). +> (b) Switch to auto mode (`/permission-mode auto`) for the duration — a classifier reviews each command and only escalates risky ones. +> (c) Ask me before every command (default — slow, not really compatible with an unattended run; confirm you actually want this). + +Concrete Bash patterns this workflow realistically needs — **all local, nothing that touches a remote**: + +- `Bash(yarn d2-style check*)` / `Bash(yarn lint*)` — touched-file and full-suite lint +- `Bash(npx jest*)` / `Bash(yarn test*)` — touched-file and full-suite tests +- `Bash(yarn build*)` — only if the plan's verification step needs it +- `Bash(yarn cy:run*)` — only if the plan touches map rendering / Cypress-covered UI; ask specifically if it comes up, don't pre-grant broadly +- `Bash(git status*)`, `Bash(git diff*)`, `Bash(git log*)`, `Bash(git add*)`, `Bash(git commit*)` — the commit cycle itself +- `Bash(git merge*)` — for syncing master into the feature branch mid-flight (merge, not rebase — team convention, see `branch-update`) +- `Skill(code-review)` / `Skill(code-review:*)` — per-step and final self-review +- Edit/Write on the repo tree (implicit, not a Bash pattern) + +**Deliberately never included here, no matter what — ask fresh, in the moment, only if actually reached (see the main SKILL.md's "Handoff" section):** + +- `Bash(git push*)` +- `Bash(gh pr create*)`, `Bash(gh pr edit*)`, `Bash(gh pr ready*)` + +This isn't a stricter default that can be relaxed by the user pre-authorizing further upfront — it's a hard boundary. Even a user who says "just go all the way through, don't stop" at the start of the run still gets a fresh, explicit ask at the actual push/PR-create moment. diff --git a/.claude/skills/manual-test-scenarios/SKILL.md b/.claude/skills/manual-test-scenarios/SKILL.md new file mode 100644 index 0000000000..c1a1cbd7ea --- /dev/null +++ b/.claude/skills/manual-test-scenarios/SKILL.md @@ -0,0 +1,35 @@ +--- +name: manual-test-scenarios +description: Use when drafting the "Manual testing" section of a PR body (or a combined write-up spanning a pr-chain) — gathers the ticket list, Netlify preview info, and real map/dashboard UIDs, then dispatches the test-scenario-writer subagent to draft the section matching this repo's real convention. +--- + +# Manual test scenarios + +Drafts the "Manual testing" section of a PR body, matching a real, consistent, already-established convention (verified across multiple real PRs) — one bullet per Jira ticket, a working link + a screenshot, not numbered click-steps. + +| Situation | Do this | +| -------------------------------------------------------------- | ------------------------------------------------------------------------------------------ | +| Single PR, tickets already known | Gather inputs (below), dispatch `test-scenario-writer` | +| A `pr-chain` — multiple PRs, each with its own Netlify preview | Ask the user: one consolidated write-up (in the tail PR) or per-PR sections? Don't assume | +| No real map/dashboard UID available yet for a scenario | Ask the user for one — never invent a UID, it has to actually exist on the pinned instance | + +## Gather inputs + +Before dispatching, collect: + +- **Ticket list** — the same `[TICKET-ID](url)` links already used under "Implements" in the PR body; reuse them verbatim, don't re-derive the URL. +- **Netlify PR number and instance** — `Netlify: https://pr-<PR#>.maps.netlify.dhis2.org/ + Instance: https://dev.im.dhis2.org/maps-app-42-3` is the real fixed pattern; confirm the PR number if not obvious. +- **Real map/dashboard UIDs** to link per scenario, from the user — this skill can't invent working test data. +- **Whether the "Dashboard tested" checklist item applies** — if so, also gather the `interpretationId` for a paired "- plugin" link alongside each "- main" link. + +## Dispatch + +Hand all of the above to the **`test-scenario-writer`** subagent to draft the actual section in the tester-facing voice (trusts a technically-comfortable internal tester with a link + screenshot, never prose click-paths). Review its draft before inserting it into the PR body — confirm every ticket has either a real test-map link or an explicit "still needs a link" placeholder, never a silently-dropped bullet. + +## For a `pr-chain` + +Each PR in the stack gets its own Netlify preview URL. Ask the user which they want: one consolidated write-up organized by ticket across the whole set (living in the tail PR), or per-PR sections each referencing that PR's own preview. Don't default to one without asking — both are reasonable and it depends on how the reviewers actually work through the chain. + +## Ties to the PR template + +This section is the evidence that lets a named tester actually check the `Tester approved (name)` Quality-checklist item — `check-tasklist.yml` blocks the PR while that box (or any other) is left unchecked with no `_N/A_`. See `commit-and-pr-messages` for the full checklist-constraint rule. diff --git a/.claude/skills/map-layer-architecture/SKILL.md b/.claude/skills/map-layer-architecture/SKILL.md new file mode 100644 index 0000000000..28279ee34c --- /dev/null +++ b/.claude/skills/map-layer-architecture/SKILL.md @@ -0,0 +1,23 @@ +--- +name: map-layer-architecture +description: Use when adding or modifying a map layer type, thematic/classification config, Earth Engine layers, or anything at the maps-app/@dhis2/maps-gl rendering boundary. +--- + +# Map layer architecture + +| Working on... | Read | +| ---------------------------------------------------------------------------------------------- | -------------------------------- | +| A layer type (thematic, event, org unit, tracked entity, facility, external, GeoJSON, basemap) | `references/layer-base-class.md` | +| Legend/classification config (color scales, legend sets, decimal places) | `references/classification.md` | +| An Earth Engine dataset layer, or a build/dev-server issue involving `@dhis2/maps-gl` | `references/earth-engine.md` | +| Behavior that differs between the standalone app and the dashboard-embedded plugin | See "Dual deployment" below | + +## The maps-app / maps-gl boundary + +maps-app owns data fetching, Redux state, and config UI. `@dhis2/maps-gl` (imported via `src/components/map/MapApi.js`) owns the actual MapLibre GL rendering, layer/control types, and the Earth Engine worker. Changing what a "layer" can do usually touches both repos — check `@dhis2/maps-gl`'s `layerTypes.js`/`controlTypes.js` registries when a new layer _kind_ is needed, not just new _config_ for an existing kind. + +## Dual deployment + +`d2.config.js` declares two entry points: `app` (`AppWrapper.jsx`, standalone) and `plugin` (`PluginWrapper.jsx`, `pluginType: 'DASHBOARD'`, embedded in a dashboard). `PluginWrapper.jsx` debounces window-resize-driven re-renders specifically for the embedded case — if you're touching resize/layout logic, check both entry points render correctly. + +**The plugin entry point does not have a Redux store.** `AppWrapper.jsx` wraps its tree in `<ReduxProvider store={store}>`; `PluginWrapper.jsx` renders `<Plugin>` directly with no `Provider` at all. Any component that needs to render inside the dashboard plugin (not just the standalone app) can't rely on `connect`/`useSelector`/`useDispatch` reaching a real store — `connect`'d components mounted under the plugin tree would break or silently get `undefined` state. Check whether a component you're changing is reachable from `Plugin.jsx` before assuming Redux is available. diff --git a/.claude/skills/map-layer-architecture/references/classification.md b/.claude/skills/map-layer-architecture/references/classification.md new file mode 100644 index 0000000000..626c7b091d --- /dev/null +++ b/.claude/skills/map-layer-architecture/references/classification.md @@ -0,0 +1,7 @@ +# Thematic mapping / classification + +`src/components/classification/` implements the legend/color-scale config UI: `Classification.jsx`, `LegendTypeSelect.jsx`, `LegendSetSelect.jsx`, `NumericLegendStyle.jsx`, `DecimalPlacesSelect.jsx`, `IsolatedClass.jsx`, `SingleColor.jsx`. This produces the `legendSet`/color-scale config consumed by `ThematicLayer.jsx` and ultimately rendered by `@dhis2/maps-gl`. + +`src/constants/colorbrewer.js` and `colors.js` back the available color-scale choices. + +This is the layer of the app most likely to have subtly-wrong edge cases (decimal place rounding, legend-set vs. ad-hoc classification, single-color vs. scaled) — when touching it, check both the "predefined legend set" and "custom classification" code paths, not just one. diff --git a/.claude/skills/map-layer-architecture/references/earth-engine.md b/.claude/skills/map-layer-architecture/references/earth-engine.md new file mode 100644 index 0000000000..21a50c70f9 --- /dev/null +++ b/.claude/skills/map-layer-architecture/references/earth-engine.md @@ -0,0 +1,9 @@ +# Earth Engine layers + +`src/constants/earthEngineLayers/*.js` declares Google Earth Engine dataset configs (heat, precipitation, vegetation, population, elevation, land cover, etc. — one file per dataset/period combination), aggregated via `src/constants/earthEngineLayers/index.js`. + +## The Vite `optimizeDeps` workaround + +`d2.config.js`'s `viteConfigExtensions.optimizeDeps.exclude: ['@dhis2/maps-gl']` exists so Vite serves `@dhis2/maps-gl` unbundled via `/@fs/...` with its full transform pipeline — this lets the Earth Engine worker's URL resolve to the actual source file rather than a `.vite/` cache path where bare imports aren't rewritten. Because `@dhis2/maps-gl` is excluded from the dep scanner, its CJS dependencies must be listed explicitly under `optimizeDeps.include` (maplibre-gl, turf packages, comlink, etc.) — if you add a new maps-gl dependency that needs bundling, it likely needs adding here too. + +If you hit a dev-server error about the Earth Engine worker failing to load or resolve, check this config block before assuming the bug is in the worker code itself. diff --git a/.claude/skills/map-layer-architecture/references/layer-base-class.md b/.claude/skills/map-layer-architecture/references/layer-base-class.md new file mode 100644 index 0000000000..635a589315 --- /dev/null +++ b/.claude/skills/map-layer-architecture/references/layer-base-class.md @@ -0,0 +1,16 @@ +# The `Layer` base class + +`src/components/map/layers/Layer.js` is an abstract React **class** component every concrete layer type extends (`ThematicLayer`, `EventLayer`, `OrgUnitLayer`, `TrackedEntityLayer`, `FacilityLayer`, `ExternalLayer`, `GeoJsonLayer`, `BasemapLayer`). It bridges React lifecycle to maps-gl's **imperative** API — don't manage layer state with plain React state, funnel changes through this base class's methods. + +- `componentDidMount` → `createLayer()` → `map.createLayer(config)` + `map.addLayer(this.layer)`. +- `componentDidUpdate(prevProps)` diffs props and picks the cheapest applicable update: + - Data/config actually changed → `updateLayer()` (destroys and recreates the underlying maps-gl layer — expensive). + - Only order changed → `setLayerOrder()`. + - Only opacity changed → `setLayerOpacity()`. + - Only visibility changed → `setLayerVisibility()`. + - A feature needs highlighting → `highlightFeature(feature)`. +- `componentWillUnmount` removes the layer from the map. + +**When adding a new layer type**: extend `Layer`, override what you need (data fetching, config shape), but reuse the base class's update-diffing rather than re-implementing it. **When modifying update behavior**: change it in the base class, not per-subclass, unless the behavior is genuinely subclass-specific. + +**Performance rule**: never trigger a full `updateLayer()` when a cheaper method covers the actual change — destroy+recreate on every prop change is the most common map-perf regression in this codebase. diff --git a/.claude/skills/mockup-pr/SKILL.md b/.claude/skills/mockup-pr/SKILL.md new file mode 100644 index 0000000000..467f599ed2 --- /dev/null +++ b/.claude/skills/mockup-pr/SKILL.md @@ -0,0 +1,122 @@ +--- +name: mockup-pr +description: Build and open a draft "mockup" PR — real, clickable code on a throwaway branch that's never intended to merge, so stakeholders can react to something concrete before the team commits to full implementation. Explicit invocation only. +disable-model-invocation: true +--- + +# Mockup PR + +| Situation | Do | +| ----------------------------------------------------- | --------------------------------------------------------------------------- | +| Haven't yet identified what this mockup is de-risking | Step 1 — ask, don't guess | +| UI/UX or information-architecture question | `references/ui-ux-fidelity.md` | +| Algorithmic/statistical feasibility question | `references/algorithmic-fidelity.md` | +| Architecture/integration feasibility question | `references/architecture-fidelity.md` | +| Ready to push + open the PR | See "Pushing and opening the PR" below — always a fresh, explicit ask first | + +A "mockup" here is a **draft, never-merged PR** whose job is to give stakeholders something they can click through on a live preview, before real implementation work starts. It is not one recipe — three real precedents in this repo (#3684, #3673, #3655) each picked a different fidelity strategy depending on _what uncertainty they were de-risking_. Picking the wrong one either wastes effort (over-building a throwaway UI mock) or fails to actually answer the open question (under-building when the risk is algorithmic or architectural). + +## Step 0 — Scope + +Before writing anything, get from the user (don't assume): + +- A one-line pitch of the feature being mocked up. +- A Jira ticket ID or link, if one already exists (there's no live Jira API access — take it as pasted text; it's fine if there isn't one yet, see PR #3673, which validated a not-yet-ticketed direction). + +## Step 1 — What is this de-risking? + +Ask (`AskUserQuestion`), single-select, always — even if it looks obvious from the user's initial description: + +> **What's the biggest open question this mockup needs to answer before real implementation starts?** +> +> - **UI/UX & information architecture** — Will this layout/workflow make sense to users? Stub data locally, no Redux/API wiring. Cheapest, most disposable. → `references/ui-ux-fidelity.md` +> - **Algorithmic/statistical feasibility** — Does the calculation actually work and perform acceptably on real data? Implement it for real, with real tests, gated by a new config flag. → `references/algorithmic-fidelity.md` +> - **Architecture/integration feasibility** — Can a new subsystem integrate with the app's real state/actions? Wire the real "hands", stub only the expensive/uncertain "brain". → `references/architecture-fidelity.md` +> - **Not sure / a mix** — talk it through before picking one. + +Don't split effort across strategies for one PR — pick the single primary uncertainty and build to answer that one. + +## Step 2 — Branch + +- Ticket exists → `mockup/<TICKET-ID>` (e.g. `mockup/DHIS2-21453`). +- No ticket yet / exploratory → `feat/<short-slug>` (e.g. `feat/spatial-analysis`, `feat/ai-agent-poc`). Branch prefix tracks "does a ticket exist," not which fidelity strategy was picked — either naming style is fine either way. + +Always branch from latest `master`. + +## Step 3 — Build + +Open the reference file matched in Step 1 and follow its guidance for what to fake and what to build real. Regardless of which strategy: use real `@dhis2/ui` components, real `core/` field-wrapper components (`src/components/core/`), real colocated `ComponentName.module.css`, real `i18n.t()` for all strings, and put files in the correct `src/components/<feature>/` location. If the mockup touches a layer type or classification/thematic config, see the `map-layer-architecture` skill for the real conventions there — don't reinvent them for the mockup. + +Commit normally as you go (`feat(scope): ...`, `fix(scope): ...`, etc. — see `commit-and-pr-messages`) — there's no special "mockup:" commit-message prefix requirement; that convention belongs to the PR title, not every commit. + +## Step 4 — Confirm before pushing + +Before running any push or PR-create command, show the user: + +- Final branch name +- The commit list (`git log master..HEAD --oneline`) +- The PR title +- The filled-in PR body (see template below) + +Get an explicit go-ahead. This isn't extra bureaucracy on top of the skill's own invocation — it's the one checkpoint before something becomes visible to the rest of the team, and per this repo's universal rule, remote writes always need a fresh, in-the-moment ask regardless of how the skill was invoked. + +## PR title + +`mockup: <short description>` — optionally append `[DHIS2-XXXXX]` if there's a ticket and the user wants it visible in the PR list. Not required even when a ticket exists (precedent: #3684 has no ticket in its title despite one in the body). + +## PR body template + +``` +Mockup{ for [<TICKET-ID>](https://dhis2.atlassian.net/browse/<TICKET-ID>)} + +DO NOT MERGE + +### Description + +**What:** <one sentence — what this demonstrates and why now> + +- <what's built> +- <what's built> +- <what's explicitly faked/stubbed and why, per the chosen fidelity strategy> +- <out-of-scope/follow-up work, named explicitly — not vague> + +--- + +### Screenshots + +<!-- Drag screenshots into the PR description on github.com after it's open — + gh/the API can't embed inline images (github.com's drag-and-drop is a + private endpoint, not part of the public REST API or gh CLI). Omitting + this section entirely is also fine and has precedent (#3655) when the + live preview is expected to carry that weight instead. --> +``` + +Drop the `{ for [...] }` bracketed clause entirely (leaving plain `Mockup`) if there's no ticket. + +## Pushing and opening the PR + +**Per this repo's universal rule: never push or open a PR without a fresh, explicit, in-the-moment ask — every time, no exceptions, regardless of how this skill was invoked.** There's no pre-provisioned write credential to reach for and no way to bundle this into an earlier approval. Confirm with the user right before running either command, then attempt it through the normal tool-permission flow (which will prompt) — don't try to route around that prompt. + +```bash +git checkout master && git pull origin master +git checkout -b mockup/DHIS2-21453 # or feat/<short-slug> + +# ... commits happen during Step 3 ... + +git push -u origin mockup/DHIS2-21453 + +gh pr create \ + --draft \ + --base master \ + --title "mockup: <short description>" \ + --label mockup \ + --body-file <path-to-pr-body.md> +``` + +`--label mockup` assumes the label exists on the target repo (it does on `dhis2/maps-app`, confirmed). If it's missing on a fork, `gh pr create` fails with `could not add label: 'mockup' not found` — surface that error rather than retrying without the label. + +## After opening + +- **Never run `gh pr ready`** on this PR — it's meant to stay in draft forever. +- A SonarQube quality-gate failure comment is expected on these PRs — don't try to clean it up; that would defeat the point of a cheap, fast mockup. +- A Netlify bot comment with the live preview URL (`pr-<n>.maps.netlify.dhis2.org`) usually appears within a couple of minutes (`gh pr view <n> --comments` to check) — this is how stakeholders click through the mockup without pulling the branch. It's not guaranteed to post every time; don't block the handoff on it. diff --git a/.claude/skills/mockup-pr/references/algorithmic-fidelity.md b/.claude/skills/mockup-pr/references/algorithmic-fidelity.md new file mode 100644 index 0000000000..383932746d --- /dev/null +++ b/.claude/skills/mockup-pr/references/algorithmic-fidelity.md @@ -0,0 +1,23 @@ +# Algorithmic/statistical feasibility fidelity + +De-risking: _does the actual calculation work, and hold up on real data?_ +Reference precedent: PR #3684, "mockup: spatial hotspot analysis for thematic layers" (branch `feat/spatial-analysis`). + +## What's real + +- The algorithm itself, in full — not a stub. Precedent: 675-line `src/util/spatialStats.js` implementing real Getis-Ord Gi\*/Local Moran's I statistics, with a 456-line Jest spec actually exercising the math. +- Wiring into the **real production pipeline** — precedent: `getGiStar`/`getLisa` called directly from `src/loaders/thematicLoader.js`, not from a side path. +- New UI reuses real `core/` field-wrapper components, real Redux actions/reducers, and the existing dialog's CSS module — this strategy isn't about faking UI plumbing, only about proving the calculation. +- A genuinely new runtime dependency if the algorithm needs one — precedent: `@turf/distance` added to `package.json`/`yarn.lock` for real, not vendored or copy-pasted. + +## What makes it safe to merge-adjacent-but-not-merge + +- **Gate the entire feature behind a new config flag** so existing saved maps/configs are provably unaffected — precedent: `config.spatialAnalysis?.method` checked in `thematicLoader.js` before any of the new code path runs. Pick a flag name scoped to the feature, not a generic `experimental` toggle. + +## Commit shape + +Granular, iterative, real conventional commits as the algorithm gets built and debugged — precedent: `feat(spatialStats): implement ... with tests`, `fix(lint): resolve eslint errors across spatial analysis files`, `fix(spatialStats): fix undefined color palette crash`, `refactor(ui): align Analysis tab with the rest of the layer edit dialog`. Don't squash this history — the granularity is itself evidence the algorithm was iterated on and debugged for real, which is the whole point of this fidelity strategy. + +## PR body notes + +Explicitly frame the PR as a proof-of-concept for a first slice of a larger piece of work, and **name the follow-ups that are deliberately out of scope** (precedent: heatmap/DBSCAN clustering, multidimensional thematic maps named as future work, not built here). This is what lets reviewers calibrate how much rigor to expect from _this_ PR specifically. diff --git a/.claude/skills/mockup-pr/references/architecture-fidelity.md b/.claude/skills/mockup-pr/references/architecture-fidelity.md new file mode 100644 index 0000000000..831706c62a --- /dev/null +++ b/.claude/skills/mockup-pr/references/architecture-fidelity.md @@ -0,0 +1,33 @@ +# Architecture/integration feasibility fidelity + +De-risking: _can a new subsystem integrate with the app's real state/actions, and is the design provider-swappable/extensible the way we're claiming?_ +Reference precedent: PR #3673, "mockup: add AI map assistant" (branch `feat/ai-agent-poc`). + +## The real/fake split — the core technique + +This strategy is a deliberate split between: + +- **The "hands" — 100% real.** Whatever dispatches into the existing app must dispatch the _actual_ actions/reducers a normal user interaction would trigger. Precedent: `src/ai/tools/*.js` (e.g. `addThematicLayer.js`, `updateLayer.js`, `removeLayer.js`) call real Redux action creators — an AI-built layer is an ordinary layer the user can then edit or delete by hand, not a parallel shadow object. +- **The "brain" — explicitly faked, and say so in a comment.** Whatever part is expensive, slow, non-deterministic, or otherwise unsuitable for a live demo gets a deterministic stand-in with a code comment stating exactly what it's replacing and why. Precedent, verbatim from `src/ai/connectors/demo.js`: + + > Deterministic demo connector — implements LLMConnector without calling any model. Matches user input against DEMO_SCRIPTS and returns scripted tool calls one at a time. The real resolver tools and executor run against the live DHIS2 instance; only the "LLM reasoning" step is replaced by this fixed plan. + + Don't skip the comment — it's what makes the fake honest to the next person reading the code, and it's what lets a stakeholder demo run without depending on API keys, cost, or model non-determinism. + +- If the point being proven is "this design is swappable across providers/backends," build **more than one real implementation** of the swappable part to prove it, alongside the one demo-safe fake. Precedent: `src/ai/connectors/anthropic.js` and `openaiCompatible.js` are both real, working API-call implementations sitting next to `demo.js`. + +## Feature-flag the entry point + +New UI lives behind an `isEnabled()`-style check, not a config-file flag — precedent: `src/ai/ui/AssistantPanel.jsx` exports `isEnabled = () => window.__DHIS2_AI_ASSISTANT__ === true || localStorage.getItem(FEATURE_FLAG) === 'true' || process.env.NODE_ENV === 'development'`. This keeps the mockup invisible in production builds while still trivially reachable for a stakeholder demo (dev mode, or a `localStorage` toggle to flip on in a deployed preview). + +## Design principles before "what" + +If the mockup is validating an architectural stance (e.g. data minimization, vendor-swappability, security boundary), lead the PR body with that architecture prose _before_ the feature description — precedent: #3673's body states its two governing principles (customer controls what leaves their server; no vendor lock-in) before describing what was actually built. Stakeholders reviewing an architecture mockup are evaluating the stance, not just the demo. + +## Commit shape + +Can be coarse — precedent: #3673 has only two commits, both plain `feat: ...`, no special mockup prefix. The granularity signal that matters for the algorithmic strategy doesn't apply here; what matters is the real/fake split being clean and well-commented. + +## No ticket required + +This strategy is a legitimate way to validate a **not-yet-ticketed** direction — precedent: #3673 references no Jira ticket anywhere. Don't insist on one. diff --git a/.claude/skills/mockup-pr/references/ui-ux-fidelity.md b/.claude/skills/mockup-pr/references/ui-ux-fidelity.md new file mode 100644 index 0000000000..0a2f936e0b --- /dev/null +++ b/.claude/skills/mockup-pr/references/ui-ux-fidelity.md @@ -0,0 +1,30 @@ +# UI/UX & information-architecture fidelity + +De-risking: _will this layout/workflow/navigation make sense to users?_ +Reference precedent: PR #3655, "mockup: sources catalogue" (branch `mockup/DHIS2-21453`). + +## What to fake + +- **All data.** Write a hand-authored `mock<Thing>.js` module (precedent: `src/components/layerSources/mockCatalogueSources.js`, 753 lines of realistic-looking static data) that the component imports directly. +- **All state that would normally be Redux.** Use local `useState` inside the component (precedent: `ManageLayerSourcesModal.jsx` holds catalogue/search/filter state locally) — do not add reducers, action types, or thunks. There is no real data plumbing in this strategy, by design. +- Persistence, favorites, "saved" state, etc. — a second mock module playing the role of a store is fine (precedent: `mockFavoritesStore.js`, 27 lines). + +## What stays real + +- `@dhis2/ui` components and this repo's real `core/` field-wrapper components for every widget — a UI/UX mockup is specifically about how _these_ components compose, so faking them defeats the purpose. +- Colocated `ComponentName.module.css` per component, same as production code. +- `i18n.t()` for every string — stakeholders and translators alike should see the real strings. +- Correct file placement under `src/components/<feature>/`. + +## Expect, and don't chase + +- A high SonarQube issue count (precedent: 88 issues incl. 13 "bugs" on #3655) — consistent with fast, disposable code, and fine here. +- No Jest coverage of the mock data path is expected or needed. + +## Commit shape + +Can legitimately be a single squashed commit (precedent: #3655 shipped as one commit, `mockup: add layer, layer sources, add layer source`) — this strategy doesn't need the granular history the algorithmic strategy does, since there's no real logic under test to bisect later. + +## PR body notes + +Screenshots are the natural fit for this strategy (there's a UI to show), but are not mandatory — #3655 shipped with none, leaning entirely on the live Netlify preview link instead. Either is fine. diff --git a/.claude/skills/pr-chain/SKILL.md b/.claude/skills/pr-chain/SKILL.md new file mode 100644 index 0000000000..c81428e655 --- /dev/null +++ b/.claude/skills/pr-chain/SKILL.md @@ -0,0 +1,217 @@ +--- +name: pr-chain +description: Use when planning, creating, or maintaining a stacked chain of dependent PRs (PR1 -> PR2 -> ... -> PRn, each branched from the previous) for one large epic. Covers slicing a plan into a 5-10 PR chain, propagating review-feedback changes down the still-open stack, and re-anchoring downstream PRs after an earlier one squash-merges to master. Creates real branches/PRs — always requires explicit invocation, never trigger this automatically. +disable-model-invocation: true +--- + +# PR chain + +A "chain" is a stack of dependent branches for one epic: `pr1 -> pr2 -> ... -> prN`, each branched from the previous one's branch (not from `master`), each its own small, independently-reviewable PR. This repo squash-merges everything, which is the one fact that makes chains tricky — see "After a stack member merges" below before touching anything post-merge. + +| Situation | Do this | +| ----------------------------------------------------------------------------------------------- | ----------------------------------------------------------------- | +| You have (or need to produce) a plan with 3-10 discrete steps and want it as a reviewable chain | "Creating the chain" | +| Review feedback changed an earlier PR that **hasn't merged yet** | "Propagating a change down the chain" | +| An earlier PR in the chain **just squash-merged to master** | "After a stack member merges" + `references/squash-merge-sync.md` | +| The chain is pushing past ~10 PRs, or a propagation is turning into a conflict-fest | "When to stop and re-plan" | +| You just want to sanity-check the chain's current shape | "Chain health check" | + +## Before creating anything + +This skill only runs when explicitly invoked — it stages branches and opens PRs, which `CLAUDE.md`'s "don't stage or commit unless explicitly asked" policy puts squarely behind explicit consent. **Every push, `gh pr create`, and `gh pr edit` in this workflow still needs its own fresh, explicit, in-the-moment ask** — per this repo's universal rule, the initial `/pr-chain` invocation authorizes the workflow conceptually, not a standing blanket permission for every subsequent remote write across what could be a long-running, multi-day epic. Confirm before each one. + +Interview the user before creating branches: + +- **Slices**: if there's an existing plan (e.g. from `implement-plan`) with numbered steps, use those as the slice boundaries and confirm the count; otherwise ask them to describe the epic and propose a slicing, then confirm it — don't invent slice boundaries unilaterally. +- **Ticket scheme**: this team typically uses **one shared epic ticket across the whole chain**, with PRs numbered `[PR1]`, `[PR2]`, ... in their titles — confirm that's what's wanted here rather than assuming a sub-ticket per slice. +- **Count check**: if the plan has more than ~10 steps, flag it before creating anything (see "When to stop and re-plan") rather than mechanically making a 14-PR chain. + +## Rollout + +**PR1 opens ready-for-review; PR2..N open as drafts incrementally**, each only once its own branch/commits actually exist — not all upfront. This keeps upfront churn low while still building the dependency graph naturally as work progresses. + +## Naming convention + +No prior real chain exists in this repo's history to copy exactly (a seeded example, `feat/datatable-pr1-toolbar` → `pr2-bidirectional-sync` → ..., confirms the merge-based sync convention but predates a ticket-based naming scheme — and also contains a decoy `feature/...` branch using the wrong, non-existent type prefix; don't copy that). This is a new convention layered onto the existing `<type>/<TICKET-ID>[-short-description][-vN]` pattern: + +``` +<type>/<TICKET-ID>-PRk-<short-description> +``` + +e.g. for a 4-PR chain against `DHIS2-18821`: + +``` +feat/DHIS2-18821-PR1-toolbar +feat/DHIS2-18821-PR2-bidirectional-sync +feat/DHIS2-18821-PR3-filtering +feat/DHIS2-18821-PR4-columns +``` + +Keeping one shared `TICKET-ID` (the epic's) across every branch in the chain makes the whole stack `grep`-able and sortable together: `git branch -a | grep DHIS2-18821-PR`. + +PR titles get the bracketed position marker matching the user's real convention (confirmed by real `[PR7] [DHIS2-18242]`-style tags already seen in this team's PRs): `[PRk] <type>: <description> [<TICKET-ID>]`. + +## Creating the chain + +Build branches strictly in order, each one rooted on the previous, pushing and opening each PR before starting the next slice (so cross-links can reference real PR numbers) — confirming with the user before each push/`gh pr create`: + +```bash +git fetch origin master + +git checkout -b feat/DHIS2-18821-PR1-toolbar origin/master +# ... implement slice 1, commit ... +# ask before pushing: +git push -u origin feat/DHIS2-18821-PR1-toolbar +# ask before opening the PR: +gh pr create --base master --head feat/DHIS2-18821-PR1-toolbar \ + --title "[PR1] feat: add data table toolbar [DHIS2-18821]" \ + --body "$(cat <<'EOF' +Implements [DHIS2-18821](https://dhis2.atlassian.net/browse/DHIS2-18821) + +### Chain + +Part 1 of 4: **#<PR1> (this PR)** -> #<PR2 once opened> + +- First in the chain — no dependency. +- Merge order matters: this must land before the rest of the chain. + +### Description +... +EOF +)" +# capture the PR number gh just printed, e.g. PR1=1234 + +git checkout -b feat/DHIS2-18821-PR2-bidirectional-sync feat/DHIS2-18821-PR1-toolbar +# ... implement slice 2, commit ... +# ask before pushing: +git push -u origin feat/DHIS2-18821-PR2-bidirectional-sync +# ask before opening the draft PR: +gh pr create --draft --base feat/DHIS2-18821-PR1-toolbar \ + --head feat/DHIS2-18821-PR2-bidirectional-sync \ + --title "[PR2] feat: add bidirectional map/table selection sync [DHIS2-18821]" \ + --body "... Chain: Part 2 of 4: #<PR1> -> **#<PR2> (this PR)** -> #<PR3 once opened> ..." +# capture PR2's number, then ask before backfilling PR1's "Followed by": +gh pr edit <PR1> --body "<PR1's body with the Chain section's 'Followed by' line filled in as #<PR2>>" +``` + +Repeat for slices 3..N, each branch based on the previous, each new PR's body naming the ones before and after it, and each time backfilling the previous PR's "Followed by" line once the new PR's number exists. The tail PR (`PRN`) has no "Followed by" line. + +Base branches are chained on purpose (`gh pr create --base <previous-branch>`, not `--base master`) — that's what makes each PR's diff show only that slice's own changes instead of the whole epic. Only PR1 targets `master`. + +## PR body: chain cross-links + +Add a `### Chain` section right after the `Implements [...]` line, before `### Description`: + +```markdown +Implements [DHIS2-18821](https://dhis2.atlassian.net/browse/DHIS2-18821) + +### Chain + +Part 2 of 4 in the DHIS2-18821 chain: #1234 -> **#1235 (this PR)** -> #1236 -> #1237 + +- Depends on: #1234 — merge that first. +- Base branch: `feat/DHIS2-18821-PR1-toolbar` (retargets to `master` once #1234 merges). +- Followed by: #1236. + +### Description + +... +``` + +**Gotcha:** `.github/workflows/check-tasklist.yml` (`Shopify/task-list-checker`) fails the PR on _any_ unchecked `- [ ]` box anywhere in the body, not just the Quality checklist section. Write the Chain section with plain `-` bullets, never `- [ ]` — an "unmerged dependency" checkbox would permanently block CI until PR1 merges and someone remembers to tick it. + +Since there's no stacked-PR tooling installed (checked: no Graphite config, no `git-branchless`, no `gh` stacking extension, no relevant git aliases in this repo), merge-order is enforced purely by this text plus reviewer discipline — GitHub itself won't block someone from merging PR2 out of order. + +## Propagating a change down the chain (all still open) + +Team convention for keeping any branch in sync is **merge, never rebase** (this repo's history is full of `Merge branch 'master' into <feature-branch>` commits, no rebases — see `branch-update`) — apply the same rule between stack members while nothing has merged to master yet: + +```bash +# PR1's branch got new commits (e.g. addressing review feedback) +git checkout feat/DHIS2-18821-PR2-bidirectional-sync +git fetch origin feat/DHIS2-18821-PR1-toolbar +git merge origin/feat/DHIS2-18821-PR1-toolbar +# resolve conflicts if any, commit the merge +# ask before pushing: +git push origin feat/DHIS2-18821-PR2-bidirectional-sync +``` + +Then cascade the same merge down the rest of the tail, one generation at a time, in order (pr2 into pr3, the _updated_ pr3 into pr4, ...) — don't skip a link. If several PRs changed around the same time, do one top-to-bottom pass rather than N separate passes. + +If a merge conflicts, resolve it in place and commit — don't rebase to dodge the conflict. Rebasing here would both break convention and complicate the eventual squash-merge fix (below), since that fix already has to reason carefully about which commits are "real." + +## After a stack member merges + +The moment PR1 squash-merges, PR2's branch has a problem: it still contains PR1's original, unsquashed commits, but `master` now has a single new squash commit instead. PR2's diff against `master` will suddenly show PR1's _entire_ changeset again on top of its own — a huge, wrong, likely-conflicting "phantom diff." This is the classic stacked-PR-plus-squash-merge failure mode. + +**Fix it immediately after the merge, before accepting further review feedback on PR2 if you can** — the phantom diff is confusing to any reviewer who opens PR2 in the meantime, and delaying lets the problem compound if PR3+ get merged down from PR2 before PR2 is fixed. + +The fix is a `git rebase --onto`, re-anchoring PR2 directly onto the new `master` and dropping everything that came from PR1 (git's default rebase already drops merge commits and replays only PR2's own real commits — this is a genuine, narrow exception to "this team doesn't rebase": once a stack member has actually merged, there is no merge-based way to make the downstream branch's diff correct again). Full mechanics, the cascading effect on PR3..PRN, and a real limitation to watch for are in `references/squash-merge-sync.md` — read that before doing this the first time. Condensed version: + +```bash +# 1. Get PR1's exact final head commit (works even if its branch was deleted post-merge — +# GitHub keeps this ref forever). The read-only GH_TOKEN can do this step (a read). +git fetch origin refs/pull/<PR1_NUMBER>/head +OLD_PR1_TIP=$(git rev-parse FETCH_HEAD) + +# 2. Re-anchor PR2 directly onto the new master +git fetch origin master +git checkout feat/DHIS2-18821-PR2-bidirectional-sync +OLD_PR2_TIP=$(git rev-parse HEAD) # keep this — PR3 needs it next +git rebase --onto origin/master "$OLD_PR1_TIP" feat/DHIS2-18821-PR2-bidirectional-sync +# resolve any conflicts, then verify: lint/test/build before pushing — see reference doc for why +# ask before pushing (force-with-lease, since history was rewritten): +git push --force-with-lease origin feat/DHIS2-18821-PR2-bidirectional-sync +# ask before retargeting the base: +gh pr edit <PR2_NUMBER> --base master + +# 3. Cascade the same treatment down the rest of the tail (PR3 onto PR2's new tip, etc.) +git checkout feat/DHIS2-18821-PR3-filtering +git rebase --onto feat/DHIS2-18821-PR2-bidirectional-sync "$OLD_PR2_TIP" feat/DHIS2-18821-PR3-filtering +# ask before pushing: +git push --force-with-lease origin feat/DHIS2-18821-PR3-filtering +# PR3's base stays pr2's branch — only the just-merged PR's immediate child retargets to master +``` + +After the cascade, the chain is back to "merge, not rebase" for any further changes among the remaining open PRs — this rebase pass is a one-time re-anchoring triggered specifically by a squash-merge, not a permanent change of technique. + +**Known limitation** (full detail in the reference doc): plain rebase drops merge commits entirely, so if a past merge commit contained a real hand-resolution (not just mechanical merging), that fix silently vanishes and won't reappear as a conflict — mitigate by running the full suite after every rebase in the cascade, treating any failure as "re-add this as a new small commit," not as a sign the technique failed. + +## Merge/review order discipline + +- Merge strictly top-down: PR1, then PR2, then PR3, ... Never merge a downstream PR first — its diff still contains every upstream PR's unsquashed commits, so merging it "early" would dump the whole epic onto `master` out of order and out of squash-granularity. +- State this in every PR body's `### Chain` section (see above) — that's the only enforcement available; there's no installed tooling that blocks out-of-order merges. +- After each merge, immediately run the "After a stack member merges" fix on the new head of the chain before anything else touches it. + +## When to stop and re-plan + +Don't mechanically keep propagating if: + +- The chain is at or approaching ~10 PRs (the user's own stated ceiling) — ship what's ready, and treat the remainder as a fresh, shorter chain rooted on the new `master` rather than growing the current one further. +- A single propagation touches most of the remaining tail with real conflicts (not clean merges) — that usually means a slice boundary was wrong (shared/foundational work leaked into a later PR instead of an earlier one). Pause and discuss re-slicing rather than resolving conflict after conflict. +- Only 1-2 PRs remain and they're small — consider whether it's still worth maintaining stack machinery versus just merging them into one PR. +- Rule of thumb: if fixing one thing requires touching more than half the remaining stack with conflicts, stop and raise it with the user instead of continuing to merge-and-resolve. + +## Chain health check + +```bash +# Find every branch in a chain by its shared ticket ID +git branch -a | grep "DHIS2-18821-PR" + +# Confirm branch k is still directly descended from branch k-1 +git merge-base --is-ancestor origin/feat/DHIS2-18821-PR1-toolbar origin/feat/DHIS2-18821-PR2-bidirectional-sync && echo ok + +# See what branch k adds on top of branch k-1 (should look like "just that slice", not the whole epic) +git log --oneline origin/feat/DHIS2-18821-PR1-toolbar..origin/feat/DHIS2-18821-PR2-bidirectional-sync + +# Confirm GitHub's recorded base matches the chain (read-only GH_TOKEN is enough for this) +gh pr view <PR2_NUMBER> --json baseRefName,number,title +``` + +If a `..` diff between adjacent branches looks like the _whole epic_ rather than one slice, that branch is out of sync with a squash-merge upstream of it — go to "After a stack member merges". + +## Related + +- `implement-plan` — a natural source of the numbered plan steps that become this chain's slices. +- `manual-test-scenarios` — organizing "Manual testing" sections across a whole chain, since each PR gets its own Netlify preview. +- `branch-update` — the simpler, single-branch version of "merge master in" this skill's own step reuses. diff --git a/.claude/skills/pr-chain/references/squash-merge-sync.md b/.claude/skills/pr-chain/references/squash-merge-sync.md new file mode 100644 index 0000000000..d66839ccdb --- /dev/null +++ b/.claude/skills/pr-chain/references/squash-merge-sync.md @@ -0,0 +1,86 @@ +# Squash-merge sync: why PR2 breaks when PR1 merges, and how to fix it + +## The failure mode, concretely + +Say `feat/DHIS2-18821-PR1-toolbar` has commits A, B, C. `feat/DHIS2-18821-PR2-bidirectional-sync` was branched from it and has its own commits D, E, plus a couple of `Merge branch 'pr1...' into pr2...` merge commits picked up while PR1 was still being revised. Its ancestry is: + +``` +master(old) - A - B - C - [merge] - D - [merge] - E <- pr2 branch tip +``` + +PR1 merges to `master` via squash. `master` now looks like: + +``` +master(old) - S <- S = squash of A+B+C, one commit +``` + +PR2's diff is computed as `git diff master...pr2-branch` (merge-base to tip). The merge-base of `master`(new) and `pr2-branch` is still `master(old)` — S is a _sibling_ of A/B/C, not a descendant of them, so `master`(new) doesn't dominate A/B/C at all. The diff therefore includes A, B, C's changes a second time (once as the squash commit's shadow, once as pr2's own inherited history) stacked on top of PR2's real D/E changes. Result: PR2 in the GitHub UI suddenly shows the entire epic's diff, usually with conflicts, even though nothing about PR2's actual intent changed. + +## Why `git rebase --onto` is the right tool here (and why it's an exception) + +This team's rule for keeping branches in sync is merge, not rebase (confirmed: this repo's history is full of `Merge branch 'master' into <feature>` commits and has no rebases). That rule works precisely because merging never changes commit identity — A/B/C keep their SHAs everywhere they're merged in, so nothing downstream ever gets confused about what's "already there." + +Squash-merging breaks that invariant on GitHub's side, not this team's: `master` gets a _new_ commit (S) with a _new_ SHA that has no ancestry relationship to A/B/C at all. There is no merge you can perform that fixes this — merging `master`(new) into `pr2-branch` would just add S alongside A/B/C, worsening the duplication rather than resolving it. The only way to make PR2's diff correct again is to give PR2 a history where A/B/C literally aren't there anymore, replaced by "master already has S." That requires rewriting pr2-branch's history — a rebase. This is a deliberate, narrow exception: rebase only at the moment a stack member has _merged_, and only to re-anchor the immediate child, never as a general substitute for the merge convention above. + +## The fix, step by step + +### 1. Get PR1's exact final commit — before its branch ref disappears + +Repos with "delete head branch on merge" enabled remove `origin/feat/DHIS2-18821-PR1-toolbar` right after merge. GitHub keeps a permanent ref for every PR regardless: `refs/pull/<PR1_NUMBER>/head`, resolvable forever, readable through the project's normal read-only `GH_TOKEN` (this is a read, not a write): + +```bash +git fetch origin refs/pull/<PR1_NUMBER>/head +OLD_PR1_TIP=$(git rev-parse FETCH_HEAD) +``` + +Equivalent alternative (also read-only-token-safe): `gh pr view <PR1_NUMBER> --json headRefOid -q .headRefOid`. + +Do this as early as possible after seeing the merge notification — don't rely on the local `feat/DHIS2-18821-PR1-toolbar` branch still being the right pointer; if PR1 got any late-breaking commits right before merge that never made it to your local checkout, the ref-based fetch is the authoritative source. + +### 2. Re-anchor PR2 onto the new master + +```bash +git fetch origin master +git checkout feat/DHIS2-18821-PR2-bidirectional-sync +OLD_PR2_TIP=$(git rev-parse HEAD) # save this before rewriting — PR3 needs it +git rebase --onto origin/master "$OLD_PR1_TIP" feat/DHIS2-18821-PR2-bidirectional-sync +``` + +What this actually does: plain `git rebase` (no `--rebase-merges`/`-p`) walks the range `$OLD_PR1_TIP..pr2-branch`, **drops every merge commit in that range entirely**, and replays only the ordinary (non-merge) commits — i.e. exactly D and E, PR2's own real work — on top of `origin/master`. Since `$OLD_PR1_TIP` is A/B/C's own tip, A/B/C themselves are excluded from the range in the first place (they're ancestors of the boundary, not part of it). No manual commit-picking is needed; this is git already doing "PR2's own commits only," automatically and correctly. + +Verify before pushing — run the full lint/test/build, not just a conflict-free `rebase --continue` (see "Known limitation" below), then, after an explicit, in-the-moment ask (this repo's universal remote-write rule — the rebase rewrote history, so this push needs `--force-with-lease`, which makes the ask especially important): + +```bash +git push --force-with-lease origin feat/DHIS2-18821-PR2-bidirectional-sync +gh pr edit <PR2_NUMBER> --base master +``` + +If GitHub already auto-retargeted PR2's base to `master` because it deleted `pr1`'s branch, `gh pr edit --base master` is a no-op — safe to run either way. Retargeting the base alone does **not** fix the diff; the rebase is what fixes the diff, retargeting the base is just bookkeeping so GitHub compares against the right side going forward. + +### 3. Cascade down the rest of the tail + +PR3 was based on PR2, but PR2's commits just got new SHAs. A merge of the rebased PR2 into PR3 would not resolve the duplication — it needs the same `--onto` treatment, using PR2's _pre-rebase_ tip (saved as `$OLD_PR2_TIP` above) as the boundary. As before, verify with the full suite before pushing, and get an explicit, in-the-moment ask before the push itself — this repo's universal remote-write rule applies to every push in this cascade, not just the first one: + +```bash +git checkout feat/DHIS2-18821-PR3-filtering +git rebase --onto feat/DHIS2-18821-PR2-bidirectional-sync "$OLD_PR2_TIP" feat/DHIS2-18821-PR3-filtering +git push --force-with-lease origin feat/DHIS2-18821-PR3-filtering +``` + +PR3's _base_ branch setting doesn't change here — it's still `feat/DHIS2-18821-PR2-bidirectional-sync`, just now pointing at that branch's new tip. Repeat the same pattern for PR4..PRN, always using the previous branch's pre-rebase tip as the boundary and its post-rebase tip as the new base. Do the whole cascade top-to-bottom in one sitting — a half-migrated stack (PR2 fixed, PR3 not) is worse than not starting, since PR3's diff is now broken in a _new_ way relative to the just-rewritten PR2. + +Once the cascade reaches the tail, resume plain merge-based propagation for any further changes among the remaining open PRs (see the main `SKILL.md`) — this rebase pass is one-time, triggered by the merge event, not an ongoing change of technique. + +## Known limitation: merge-commit-only resolutions get silently dropped + +Because plain rebase drops merge commits from the replay list entirely, if a past `Merge branch 'pr1' into pr2` was more than a mechanical merge — e.g., it hand-resolved a real conflict by updating a call site for a function PR1 renamed — that resolution lives _only_ in the merge commit's own diff, not in any of D/E. Dropping the merge commit drops that fix too, and because the surrounding text often still applies cleanly against the new base, this usually does **not** show up as a rebase conflict — it just silently reverts, and you find out later from a broken build or a failing test. + +Mitigation: after every rebase in this cascade, before pushing, run the full build/lint/test suite — don't rely on "the rebase completed without conflicts" as a correctness signal. Treat any resulting failure as "this logic needs to be re-added as a new small commit on top," not as evidence the technique is wrong. + +## Why not the alternatives + +| Technique | Verdict | +| ------------------------------------------------------------------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `git rebase --onto <new-master> <old-PR1-tip> <branch>` (chosen) | Automatically replays exactly the downstream branch's own non-merge commits onto the new master. Same result as manual cherry-picking, with git doing the sequencing. | +| Manually recreate the branch from `master` and cherry-pick each of PR2's own commits | Produces an identical result to `rebase --onto` (same commit set, same order), but requires hand-enumerating `git log --no-merges <old-PR1-tip>..pr2-branch` and cherry-picking one at a time — more manual bookkeeping for no behavioral difference. Worth falling back to only if you also want to interactively squash/reorder PR2's own fixup commits (e.g. folding "chore: sonarqube issues" into the commit it's fixing) while you're already rewriting history anyway. | +| `git rebase --rebase-merges --onto ...` | Reconstructs the merge topology instead of flattening it — which means it _replays_ the old `Merge branch master into pr1`/`Merge branch pr1 into pr2` merges too, reintroducing exactly the duplicate-content conflict against the new master that this whole exercise exists to avoid. Does not fix the problem. | diff --git a/.claude/skills/pr-polish/SKILL.md b/.claude/skills/pr-polish/SKILL.md new file mode 100644 index 0000000000..ce2f9197fa --- /dev/null +++ b/.claude/skills/pr-polish/SKILL.md @@ -0,0 +1,66 @@ +--- +name: pr-polish +description: Use when iterating on an already-open PR across multiple rounds of manual testing and feedback — organizing fix lists, re-verifying each fix narrowly, and deciding when to hand off to the pre-review skill for the final self-check. Heavier and more interactive than pre-review, which is only the last quick pass. +--- + +# PR polish + +The umbrella workflow for an open PR (yours, or handed off from another Claude session) that isn't done after one pass — the user manually tests, reports back a list of things to fix, Claude fixes them, and this repeats until the user is satisfied. `pre-review` is the last, quick step of this loop, run once per "I think we're done" moment — not a replacement for it. + +| Situation | Do this | +| -------------------------------------------------------------- | ------------------------------------------------------------------------------------ | +| Picking this PR up cold (new session, or another agent's work) | Orient first — step 1 | +| User just handed you a list of desired fixes | Turn it into a todo list — step 2 | +| A fix from the todo list is implemented | Verify narrowly, not the whole suite — step 3 | +| Todo list is fully checked off | Report back and wait for the next round of manual testing — don't assume you're done | +| User says something like "I think this is it" / "ship it" | Run the `pre-review` skill — step 4 | +| `pre-review`'s self-review surfaces a new issue | Loop back to step 2 — see "Repeated final passes" | + +## 1. Orient + +Before touching anything, confirm what state the PR is actually in — don't assume your session's memory matches reality, especially if earlier rounds happened without you: + +``` +gh pr view --json number,title,body,state +gh pr diff +``` + +Skim the diff and the PR body's ToDos/Known issues sections; don't re-litigate decisions already reflected there. + +## 2. Turn feedback into a todo list + +When the user gives you a batch of fixes (from manual testing, a review comment, a screenshot, whatever), convert each distinct item into a `TodoWrite` entry before starting on any of them — this keeps the list visible and resumable across a long session, the same pattern the `sonarqube-fix` command uses for its issue list. Word each item as a concrete, checkable outcome ("org unit dialog closes on Escape"), not a vague restatement ("fix the dialog"). + +If an item is ambiguous, ask rather than guessing — this workflow exists because manual testing catches things automated tests don't, so precision matters more than speed here. + +## 3. Fix, then verify narrowly + +For each todo: + +1. Implement the fix. +2. Verify just that fix — the touched file(s), per `CLAUDE.md`'s "Lint/test workflow for agents" (`npx jest <file>`, `yarn d2-style check <file>`). Don't run the full `yarn lint && yarn test` after every single item; that's step 4's job. +3. Mark the todo complete and tell the user _specifically_ what changed and what to re-check — manual testing only catches regressions if the user knows where to look. + +Run the full suite after a batch (roughly 3-5 related fixes, or whenever the todo list empties) — same cadence as `sonarqube-fix`. + +Don't commit as you go unless the user asks — `CLAUDE.md`'s "don't stage or commit unless explicitly asked" applies throughout this loop, not just at the very end. + +### Keeping the PR body honest (optional, ask first) + +If a Quality checklist item becomes true (tests added, dashboard tested, etc.), propose the updated checklist text to the user. Never leave a box unchecked that's actually done, and never leave `- [ ]` on something that turned out not to apply — replace it with `_N/A_` per the template's own instruction. `check-tasklist.yml` blocks the PR while _any_ unchecked box exists anywhere in the body, so an accurate checklist matters more here than in most repos. + +Editing the PR body is a remote write (`gh pr edit`) — per this repo's universal rule, propose the text and get an explicit, in-the-moment ask before applying it, every time. Never apply it just because the checklist item is objectively true now. + +## 4. Know when it's actually done + +Finishing the current todo list means "ready for another round of manual testing," not "ready for review." Only move to the final pass when the user explicitly confirms the current round passed clean — then invoke `pre-review`. + +### Repeated final passes + +`pre-review`'s self-review step can itself surface a new issue. When it does: add it as a new todo (step 2), fix and verify it (step 3), then re-run `pre-review` from the top — a changed diff needs a fresh lint/test/self-review pass, not a partial re-check of just the new lines. If you're on a third or later `pre-review` pass for the same PR, say so explicitly — that's often a sign of a design question worth discussing rather than another quick fix. + +## See also + +- `pre-review` — the final quick self-check this workflow hands off to once the user confirms a round of testing passed. +- `sonarqube-fix` — the same todo-list-per-batch pattern, applied to SonarCloud findings instead of user-reported fixes. +- `commit-and-pr-messages` — format rules for the PR title/description this skill helps keep accurate. diff --git a/.claude/skills/pre-review/SKILL.md b/.claude/skills/pre-review/SKILL.md new file mode 100644 index 0000000000..59e4df2c8f --- /dev/null +++ b/.claude/skills/pre-review/SKILL.md @@ -0,0 +1,56 @@ +--- +name: pre-review +description: Use before converting a PR from draft to "ready for review" — the final quick self-check so issues get caught before a human reviewer spends time on them. Verifies tests (Jest and Cypress), SonarQube, code quality/conventions, and the PR title/description, in that order. +--- + +# Pre-review + +Still iterating on feedback from manual testing? Use `pr-polish` instead — come back here once a round of testing passes clean; this is the last, quick pass, not the place to work through a fix list. + +Before marking a PR ready for review and requesting a human review, verify all four in order: + +## 1. Zero failing tests + +``` +yarn lint && yarn test +``` + +Plus any relevant `yarn cy:run <spec>` if the change touches map rendering or other Cypress-covered UI. Don't proceed past this step with anything red. + +## 2. Zero remaining SonarQube issues + +Query the same anonymous SonarCloud endpoint `/sonarqube-fix` uses: + +``` +curl -s "https://sonarcloud.io/api/issues/search?projectKeys=dhis2_maps-app&pullRequest=<pr-number>&resolved=false&ps=100" +``` + +If anything's still open, fix it now in the same BLOCKER→INFO order `/sonarqube-fix` uses, or explicitly hand off to that command if the list is long. Don't mark ready with open issues on this PR. + +## 3. Code quality + +Self-review the diff — either invoke the `/code-review` skill, or fetch it directly (`gh pr diff`) and read it critically. Specifically check for, not just "does it look reasonable": + +- **DRY** — logic duplicated across files/components that should share a helper. +- **Convention adherence** — matches this repo's real patterns (`CLAUDE.md`, and the `map-layer-architecture` skill for anything layer-related), not a plausible-but-different approach. +- Correctness bugs and genuine simplification opportunities — not style nits. + +## 4. PR title/description + +Complete, succinct, "what"-focused (not overlong on "why"/"how"), and matches `.github/pull_request_template.md`'s structure exactly — see the `commit-and-pr-messages` skill for the format rules rather than re-deriving them here. Every `- [ ]` in the body must be genuinely expected to complete or replaced with `_N/A_` — `check-tasklist.yml` blocks the PR on any unchecked box left anywhere in it, not just the Quality checklist section. + +## 5. Check CI, then mark ready + +``` +gh pr checks +``` + +Don't mark ready while checks are still running or failing. Marking ready (`gh pr ready`) is a remote write — per this repo's universal rule, this always needs a fresh, explicit, in-the-moment ask before running it, regardless of how this skill was invoked; never treat "the user asked for a pre-review" as blanket permission to also mark the PR ready without asking separately. + +If `gh` isn't set up (no `GH_TOKEN` configured yet), use `curl` against the public GitHub REST API instead: + +- Diff: `curl -s -H "Accept: application/vnd.github.v3.diff" "https://api.github.com/repos/{owner}/{repo}/pulls/{n}"` +- Checks: `curl -s "https://api.github.com/repos/{owner}/{repo}/commits/{sha}/check-runs"` +- PR number for the current branch: `curl -s "https://api.github.com/repos/{owner}/{repo}/pulls?head={owner}:{branch}&state=open"` + +`{owner}/{repo}` comes from `git remote get-url origin`; `{branch}` from `git rev-parse --abbrev-ref HEAD`. diff --git a/.claude/skills/spec-from-ticket/SKILL.md b/.claude/skills/spec-from-ticket/SKILL.md new file mode 100644 index 0000000000..a6bf9dea02 --- /dev/null +++ b/.claude/skills/spec-from-ticket/SKILL.md @@ -0,0 +1,42 @@ +--- +name: spec-from-ticket +description: Use when the user pastes Jira ticket content (title/description/acceptance criteria) and asks for a durable spec or implementation-plan document — phrases like "spec this out", "write a spec for this ticket", "turn this into a plan doc", "I want to hand this to another session/agent". Interviews on genuinely ambiguous points, explores the codebase via a subagent first, then authors a self-contained document to .claude/specs/. Don't use for a same-session "just implement it" request, or a quick approve-and-go plan — that's Plan Mode. +--- + +# spec-from-ticket + +Turns a pasted Jira ticket into a **durable, standalone spec + implementation plan** — meant to be picked up cold, later, by a human or a different AI session, with no memory of this conversation. + +| Situation | Do this | +| -------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------- | +| User pasted ticket content and wants a spec/plan doc/handoff artifact | Run the flow below | +| User pasted ticket content and just wants it implemented now, this session | Skip this skill — implement directly, or use Plan Mode if you want sign-off before executing | +| A `.claude/specs/<TICKET-ID>-*.md` already exists for this ticket | Read it first, update it via the `spec-writer` subagent — don't create a duplicate | +| A spec produced by this skill is ready to build | Hand it to the `implement-plan` skill — don't re-interview | + +No Jira/Atlassian connector is authorized here. Work only from what the user pastes into the conversation — never guess at fields the ticket didn't mention. + +## 1. Capture the ticket + +Note the ticket ID in the real bracket format used elsewhere in this repo's commits/branches: `PROJECTKEY-NUMBER` (e.g. `DHIS2-20564`, `CLIM-501`). If the user didn't paste one, ask for it once; if there genuinely isn't one, proceed ticket-less. + +## 2. Explore before you interview + +Dispatch a plain Explore subagent with the pasted ticket content and ask it to report back: concrete files/components/actions/reducers likely touched, existing patterns to follow (check the `map-layer-architecture` skill if a layer is involved), and anything it _couldn't_ resolve from code alone. Do this in a subagent so the broad reading doesn't burn the main session's context — you only need the findings, not the search process. + +Use those unresolved points to sharpen step 3 — don't ask the user something the exploration already answered. + +## 3. Interview — only what changes the plan + +See `references/interview-and-scoping.md` for the full guidance on what to ask vs. assume. Short version: ask about real technical forks (2+ reasonable implementations), unstated scope boundaries, and edge cases the AC is silent on. Batch questions via `AskUserQuestion`. Don't ask anything answerable by reading the code, or anything with one obviously-sane default — state the default as an assumption in the spec instead. + +## 4. Write the spec + +Dispatch the **`spec-writer`** subagent with the ticket content, the interview answers from step 3, and the Explore findings from step 2. Its job is authoring the actual document to `.claude/specs/<TICKET-ID>-<slug>.md` (`.claude/specs/<slug>.md` if ticket-less) — not `docs/`, which is the published end-user manual, a different audience entirely. Review its draft before presenting it to the user; check the "Implementation plan" section is broken into steps small and file-scoped enough that each could plausibly be one commit — that's what lets `implement-plan` consume the plan mechanically later. + +Re-running this skill against a ticket that already has a spec updates that file in place via the same subagent — never create a `-2`. + +## Handoff + +- **Plan Mode vs this skill**: Plan Mode's plan is ephemeral and session-scoped — "I approve this, execute it now." This skill's output is a durable artifact — designed to be read cold, possibly days later, possibly by a different agent entirely. Use Plan Mode when the user is at the keyboard right now and ready to go immediately; use this skill when the work might not start immediately, or the implementer isn't known yet. +- **Feeding forward**: once written, point the user at the `implement-plan` skill (`/implement-plan .claude/specs/<file>.md`) to execute it, or open a fresh Plan Mode session against the spec file directly. Either way, don't re-run the interview — the spec already captured the decisions. diff --git a/.claude/skills/spec-from-ticket/references/interview-and-scoping.md b/.claude/skills/spec-from-ticket/references/interview-and-scoping.md new file mode 100644 index 0000000000..42c5b035b4 --- /dev/null +++ b/.claude/skills/spec-from-ticket/references/interview-and-scoping.md @@ -0,0 +1,24 @@ +# Interview and scoping guidance + +## Order of operations + +1. Explore first (subagent), interview second. The interview should be _informed_ by what the code actually looks like — "I see two existing dialogs use pattern X vs. pattern Y for this, which do you want?" beats a generic open-ended question. +2. If the interview surfaces a decision that changes which files are affected, do a second, narrower Explore pass before handing off to `spec-writer` — again via subagent, so it doesn't cost the main session's context. +3. Hand off to `spec-writer` last, once decisions are made. + +## What to ask + +- **Real technical forks**: at least two reasonable implementations exist, and the ticket doesn't pick one (new layer type vs. config on an existing layer; Redux thunk vs. local component state). +- **Unstated scope boundaries**: does this need to work in the dashboard plugin (no Redux store — see `map-layer-architecture` skill) as well as the standalone app? One layer type or all of them? +- **Edge cases the AC is silent on**: empty states, permission/authority checks, what happens on API failure. +- **Tradeoffs against repo norms**: e.g. this app is in "live, released app" mode per `CLAUDE.md` — prefer minimal diffs over opportunistic refactors. If a ticket's ask nudges toward a bigger refactor than strictly needed, surface that tradeoff explicitly rather than silently picking the bigger scope. + +## What NOT to ask + +- Anything answerable by reading the code — that's what the Explore pass is for. +- Anything already stated in the ticket description or AC. +- Generic questions with one obviously-sane default — write the default into the spec's "Assumptions" section instead of interrupting the user. Reserve `AskUserQuestion` for things where a wrong guess would be expensive to unwind. + +## Batching + +Use `AskUserQuestion` with multiple questions in one call rather than serial round-trips. Stop interviewing once the remaining unknowns are cheap to fix later (i.e. would just mean editing the spec, not re-architecting). diff --git a/.gitignore b/.gitignore index dc3f7379f7..5accde5e2f 100644 --- a/.gitignore +++ b/.gitignore @@ -18,3 +18,7 @@ src/locales .idea .vscode maps-app.code-workspace + +# Claude Code (personal/runtime, not shared config) +.claude/settings.local.json +.claude/worktrees/ diff --git a/.mcp.json b/.mcp.json new file mode 100644 index 0000000000..da3aff47cf --- /dev/null +++ b/.mcp.json @@ -0,0 +1,17 @@ +{ + "mcpServers": { + "chrome-devtools": { + "command": "npx", + "args": [ + "chrome-devtools-mcp@latest", + "--chromeArg=--use-angle=swiftshader-webgl", + "--chromeArg=--enable-unsafe-swiftshader", + "--chromeArg=--disable-dev-shm-usage" + ] + }, + "grep": { + "type": "http", + "url": "https://mcp.grep.app" + } + } +} diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000000..6a88f5ca19 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,103 @@ +# maps-app + +DHIS2 Maps app — React 18 (JSX), DHIS2 App Platform (`@dhis2/cli-app-scripts` v12 — Vite-based under the hood despite the `d2-app-scripts` CLI name), Redux + `redux-thunk` for state, `@dhis2/ui`, Jest, Cypress e2e. Core map libs: `@dhis2/maps-gl` (rendering engine), `@dhis2/analytics`, `d3-*`, `@turf/*`. + +## Stage + +Live, released app (v101.16+). Prefer minimal, targeted diffs over opportunistic refactors — e.g. leave class-component map layers as classes, don't convert to hooks incidentally. + +## Generated — never hand-edit + +- `.d2/` — build output. +- `src/locales/` (incl. `src/locales/index.js`) — generated by `d2-i18n-generate`, gitignored. + +## Structure + +``` +src/ + actions/ action creators (plain thunks, e.g. layers.js, orgUnits.js) + components/ map/, classification/, orgunits/, trackedEntity/, app/, ... + constants/ actionTypes.js, colors.js, earthEngineLayers/ + hooks/ + loaders/ + reducers/ + store/ index.js — createStore + redux-thunk + styles/ + util/ + __tests__/ +AppWrapper.jsx standalone app entry +PluginWrapper.jsx dashboard-plugin entry (debounced resize handling) +``` + +## Commands + +`yarn start` / `build` / `test` / `test:coverage` / `lint` / `format` / `cy:open` / `cy:run` / `deploy` (Yarn 1 classic). + +## Conventions + +- Comments: prefer self-documenting code. Comment only non-obvious domain context (e.g. `d2.config.js`'s Vite `optimizeDeps` comment explaining the Earth Engine worker resolution). No historical/time-bound comments. +- Relative imports (no path-alias convention). Default exports and PropTypes are the norm on class components. +- Map layers are **class components** extending `Layer` (`src/components/map/layers/Layer.js`) — follow that pattern for changes there, don't convert to hooks. New non-layer UI: functional components + hooks. +- State: Redux with plain action creators (`src/actions/`), string constants in `src/constants/actionTypes.js`, `connect`/`useSelector`/`useDispatch` from `react-redux`. **Gotcha**: the dashboard-plugin entry point (`PluginWrapper.jsx`) has no `ReduxProvider` — components reachable from there can't rely on the store. See `map-layer-architecture` skill. + +## Testing + +Jest, co-located `__tests__/*.spec.js`. + +## DHIS2 specifics + +- i18n via the generated locale bundle (see Generated, above). +- Cypress e2e reads gitignored `cypress.env.json` for `dhis2BaseUrl`/credentials. +- Researching an unfamiliar DHIS2 Web API endpoint or response shape: see the `dhis2-web-api-research` skill. + +## Plugins / MCP + +`context7`, `chrome-devtools-mcp` plugins enabled. + +**Two chrome-devtools MCP registrations exist — know which to use.** The plugin's default launch has zero configurable Chrome flags (its manifest hardcodes `npx chrome-devtools-mcp@1.6.0`). This app needs specific flags for headless WebGL (`--use-angle=swiftshader-webgl --enable-unsafe-swiftshader --disable-dev-shm-usage`), which live in the project's own `.mcp.json` `chrome-devtools` server. **Use that one (`mcp__chrome-devtools__*` tools) for anything touching the map canvas.** The plugin stays enabled anyway for its bundled skills (a11y-debugging, LCP, memory-leak). + +## Git / PR workflow + +Don't stage or commit unless explicitly asked. Use `gh` (`gh pr view`, `gh pr diff`, `gh pr checks`, `gh api ...`) for reading PR state/diff/CI — a read-only-scoped token is configured (see README), so this is safe. If `gh` isn't set up yet, fall back to plain `curl` against the public GitHub REST API. Before marking a PR ready for review: see the `pre-review` skill. + +**Universal rule — no remote writes by default, for any skill.** Never push to a remote, or open/edit/ready a PR, without a fresh, explicit, in-the-moment ask from the user for that specific action — every time, no exceptions. This is never pre-negotiated or bundled into an earlier approval (not even "I approved this whole workflow up front"), and there's no special write-scoped credential to provision to get around the ask — a skill just stops and asks, then attempts the action through the normal tool-permission flow. Local operations (edit, lint, test, `git commit`, `git merge`) stay governed by "don't stage or commit unless explicitly asked" above, which an explicit workflow invocation can reasonably satisfy — remote writes cannot be satisfied that way, ever. + +## Performance + +Layer updates flow through `Layer.componentDidUpdate`'s prop diffing — prefer cheap in-place calls (`setLayerOpacity`/`setLayerVisibility`/`setLayerOrder`) over a full `updateLayer()` destroy+recreate. + +## Domain knowledge + +The maps-app ↔ `@dhis2/maps-gl` seam is `src/components/map/MapApi.js`: this repo owns data/Redux/config UI, maps-gl owns rendering. For the `Layer` base class, classification/thematic mapping, and Earth Engine details: see the `map-layer-architecture` skill. + +## Skills in this repo + +`.claude/skills/<name>/SKILL.md`, loaded on demand (not always in context). Each opens with a short decision table and pushes long reference material into `references/*.md` — keep new skills in that shape. + +- `dhis2-web-api-research` — researching unfamiliar DHIS2 API shapes. +- `map-layer-architecture` — Layer base class, classification, Earth Engine, dual app/plugin deployment. +- `pre-review` — self-review pass before marking a PR ready. +- `spec-from-ticket` — turns a pasted Jira ticket into a durable spec + implementation plan under `.claude/specs/`. +- `implement-plan` — autonomously executes a written plan through a commit-by-commit cycle. +- `mockup-pr` — builds a throwaway-but-real, never-merged draft PR for stakeholder feedback. +- `pr-polish` — iterating on manual-testing feedback across multiple rounds; hands off to `pre-review` for the final pass. +- `commit-and-pr-messages` — Conventional Commits / PR title / PR description conventions. +- `branch-update` — merge master into a feature branch and resolve conflicts (explicit invocation only). +- `pr-chain` — plans/creates/maintains a stacked chain of dependent PRs for a big epic. +- `manual-test-scenarios` — drafts the "Manual testing" section of a PR body. +- `community-post` — drafts a DHIS2 Community of Practice release-announcement post. +- `docs-update` — keeps `docs/src/*.md`/`docs/maps.md` in sync with a user-facing change. +- `claude-stack-retro` — at a work-session checkpoint, retrospects on that stretch and proposes updates to this repo's own `.claude/` tooling without editing it directly; logs decisions under `.claude/retros/`. + +## Subagents in this repo + +`.claude/agents/<name>.md` — a persona for output whose audience isn't "developer reading code," dispatched by the skill that gathers the relevant facts first. Restricted `tools:` per persona. + +- `community-post-writer` — general-public/non-technical Community of Practice voice; dispatched by `community-post`. +- `test-scenario-writer` — internal QA/tester voice; dispatched by `manual-test-scenarios`. +- `docs-writer` — end-user manual voice for `docs/src/*.md`; dispatched by `docs-update`. +- `spec-writer` — technical/architecture voice for a durable spec; dispatched by `spec-from-ticket` after its interactive interview. + +## Lint/test workflow for agents + +Touched-files-only during development: `npx jest <file>`, then `yarn d2-style check <file>` (wraps eslint+prettier, and stylelint once a local config exists — this repo doesn't have one yet, so CSS only gets prettier-checked for now). Full `yarn lint && yarn test` before finishing. The PostToolUse hook auto-formats/fixes on Edit/Write only — Bash-created edits aren't covered, run `yarn d2-style apply <file>` manually after those. diff --git a/README.md b/README.md index f7ca725864..e2afdc5de8 100644 --- a/README.md +++ b/README.md @@ -46,6 +46,19 @@ To record tests in Cypress Cloud, you can use one of the following methods based This setup helps in managing Cypress Cloud credits more efficiently, ensuring recordings are only made when explicitly required. +## Claude Code Setup (optional) + +One-time setup, for a smoother experience: + +1. Install [`jq`](https://jqlang.org/) — the auto-format/lint hook run after every file edit depends on it. +2. Install the [GitHub CLI](https://cli.github.com/) (`gh`) — most of the PR-related skills (`pre-review`, `pr-polish`, `mockup-pr`, `pr-chain`, `branch-update`, ...) use it to read (and, only on your explicit go-ahead in the moment, write) PR/CI state. Without it, those skills fall back to plain `curl` against the public GitHub REST API, which only covers reads. +3. Export a read-only-scoped `GH_TOKEN` (a [fine-grained PAT](https://github.com/settings/personal-access-tokens/new) with just Pull requests/Contents/Actions: Read) so `gh` can read PRs/CI without ever being able to write, merge, or admin on its own. +4. Run `/plugin install context7@claude-plugins-official` then `/reload-plugins` in Claude Code (chrome-devtools-mcp is already enabled via the committed settings). + +`curl` (used as the read-only fallback whenever `gh` isn't set up, and for the tokenless SonarCloud queries in `pre-review`/`sonarqube-fix`) is virtually always preinstalled — nothing to do there. `git`, `yarn`, and `node`/`npx` are already prerequisites for this repo generally (see above), not something new for the Claude Code tooling specifically. + +`CLAUDE.md` and `.claude/skills/` are read automatically — see CLAUDE.md's "Skills in this repo" and "Subagents in this repo" sections for the current list (kept there, not duplicated here, so it doesn't go stale as the tooling grows). + ## Learn More You can learn more about the platform in the [DHIS2 Application Platform Documentation](https://platform.dhis2.nu/).