Skip to content

Commit 57a5560

Browse files
leonardomendixleonardomendix
committed
chore: add code review skill
1 parent 47d6b82 commit 57a5560

2 files changed

Lines changed: 295 additions & 24 deletions

File tree

‎.claude/skills/code-review/SKILL.md‎

Lines changed: 288 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,288 @@
1+
---
2+
name: code-review
3+
description: Review a pull request in the mendix/web-widgets monorepo. Checks Mendix widget conventions, React/MobX patterns, versioning, test coverage, Atlas UI styling, security, and accessibility.
4+
---
5+
6+
# Code Review
7+
8+
Review the PR diff against the standards in this repository. Read `AGENTS.md` for full repo context.
9+
10+
## What to check on every PR
11+
12+
### PR metadata
13+
14+
- **Title**: JIRA format `[XX-000]: description` or conventional commits (`feat:`, `fix:`, etc.)
15+
- **Template adherence**: lint/test run locally, new tests added, related PRs linked
16+
- **Multi-package PRs**: validate each changed package separately
17+
18+
### Versioning and changelog (per changed package)
19+
20+
If runtime code, public API, XML schema, or behavior changed:
21+
22+
- Require semver bump in `package.json`
23+
- Require `CHANGELOG.md` entry (Keep a Changelog format)
24+
- Suggest: `pnpm -w changelog`
25+
26+
If refactor/docs/tests-only: bump not required — confirm with author.
27+
28+
### Mendix-specific
29+
30+
- **XML ↔ TSX alignment**: lowerCamelCase keys, TS props updated with XML changes, unique widget ID
31+
- **Data API**: check `ActionValue.canExecute` before `execute()`, use `EditableValue.setValue()` for two-way binding, render loading/empty states until values are ready
32+
33+
### React
34+
35+
- **Hooks**: correct `useEffect`/`useMemo`/`useCallback` deps; no stale closures; guard async effects:
36+
```ts
37+
useEffect(() => {
38+
let active = true;
39+
(async () => {
40+
const data = await load();
41+
if (active) setData(data);
42+
})();
43+
return () => {
44+
active = false;
45+
};
46+
}, [load]);
47+
```
48+
- **State**: functional updates (`setX(x => x + 1)`); no mirroring props in state without sync logic; stable `key`s in lists (not array index)
49+
- **Props**: don't spread unknown props onto DOM nodes; prefer composition over prop drilling
50+
51+
### MobX
52+
53+
- `makeAutoObservable` or `makeObservable` in every store constructor
54+
- State mutations only inside `action`; `computed` must be pure (no side effects)
55+
- React integration via `observer` HOC or `useSubscribe()` from `@mendix/widget-plugin-mobx-kit`
56+
57+
### Styling
58+
59+
- SCSS only — no inline styles for static design
60+
- Atlas UI classes preferred (`btn`, `badge`); never override core Atlas classes
61+
- BEM-like naming prefixed with widget name; no `!important`
62+
63+
### Unit tests
64+
65+
Files live in `src/**/__tests__/*.spec.ts(x)` and run with Jest + RTL (enzyme-free).
66+
67+
**Structure**
68+
69+
- Use `describe`/`it` blocks; group related cases under a nested `describe`
70+
- Define a `defaultProps` constant and a factory render helper to avoid repetition:
71+
```ts
72+
const defaultProps: MyWidgetProps = { ... };
73+
const renderWidget = (props = defaultProps) => render(<MyWidget {...props} />);
74+
```
75+
76+
**Mendix data mocking — always use builders, never manual objects**
77+
78+
```ts
79+
import { EditableValueBuilder, ListValueBuilder, actionValue, obj } from "@mendix/widget-plugin-test-utils";
80+
81+
const value = new EditableValueBuilder<string>().withValue("hello").build();
82+
const readOnly = new EditableValueBuilder<string>().withValue("x").isReadOnly().build();
83+
const loading = new EditableValueBuilder<string>().isLoading().build();
84+
const list = new ListValueBuilder().withItems([obj("A"), obj("B")]).build();
85+
const action = actionValue(); // jest.fn() — assert with .execute toHaveBeenCalled()
86+
```
87+
88+
**What to cover**
89+
90+
- All Mendix data states: `Available`, `Loading`, `Unavailable`, `ReadOnly`
91+
- All prop/behavior branches (null checks, conditional renders, edge cases)
92+
- User interactions via `fireEvent` or `userEvent`
93+
- Verify `setValue` / `execute` calls: `expect(action.execute).toHaveBeenCalled()`
94+
- Accessibility assertions: `getByRole`, `getByLabelText`, ARIA attributes
95+
96+
**What to flag**
97+
98+
- Manual mock objects instead of builders — brittle and miss status edge cases
99+
- Enzyme patterns (`shallow`, `mount`, `instance()`) — must use RTL
100+
- Snapshot tests on dynamic content (dates, IDs, async state) — use specific assertions instead
101+
- Missing `afterEach` mock cleanup — causes test pollution
102+
- Missing `ResizeObserver` / `window.mx` global setup when widget requires it (add to `jest.setup.ts`)
103+
- Jest config not extending `@mendix/pluggable-widgets-tools/test-config/jest.config`
104+
105+
### E2E tests
106+
107+
Files live in `e2e/*.spec.js` and run with Playwright (Chromium). Config inherits from `@mendix/run-e2e/playwright.config.cjs`.
108+
109+
**Mandatory structure — every file must have this**
110+
111+
```js
112+
import { test, expect } from "@playwright/test";
113+
114+
test.afterEach("Cleanup session", async ({ page }) => {
115+
await page.evaluate(() => window.mx.session.logout());
116+
});
117+
118+
test.describe("WidgetName", () => {
119+
test.beforeEach(async ({ page }) => {
120+
await page.goto("/");
121+
await page.waitForLoadState("networkidle");
122+
});
123+
});
124+
```
125+
126+
**Selectors — in order of preference**
127+
128+
1. `.mx-name-*` — Mendix widget names (most stable): `page.locator(".mx-name-myWidget")`
129+
2. ARIA roles: `page.getByRole("button", { name: "Save" })`
130+
3. Widget CSS classes: `page.locator(".widget-badge-button-text")`
131+
4. Avoid brittle selectors: nth-child chains, deeply nested CSS, text-only locators
132+
133+
**Assertions**
134+
135+
```js
136+
await expect(page.locator(".mx-name-myWidget")).toBeVisible();
137+
await expect(page.locator(".badge")).toContainText("New");
138+
await expect(page.locator(".mx-name-myWidget")).toHaveScreenshot("myWidget-default.png");
139+
```
140+
141+
**Accessibility scanning (use for new interactive widgets)**
142+
143+
```js
144+
import AxeBuilder from "@axe-core/playwright";
145+
const results = await new AxeBuilder({ page }).analyze();
146+
expect(results.violations).toEqual([]);
147+
```
148+
149+
**What to flag**
150+
151+
- Missing `afterEach` session logout — will exceed Mendix's 5-session license limit in CI
152+
- `page.waitForTimeout()` / hardcoded `sleep` — replace with `waitForLoadState` or a Playwright locator assertion
153+
- Selectors that don't use `.mx-name-*` when a Mendix widget name is available
154+
- Screenshot baselines not committed — `toHaveScreenshot` requires a baseline PNG in the repo
155+
- Missing `page.waitForLoadState("networkidle")` in `beforeEach` — causes flaky tests
156+
- E2E file not following `WidgetName.spec.js` naming convention
157+
158+
### Security
159+
160+
- **No `dangerouslySetInnerHTML`** unless input is sanitized with a trusted library (e.g. DOMPurify); flag any unsanitized usage as high severity
161+
- **No secrets or tokens** hardcoded in source — API keys, client IDs, URLs with credentials
162+
- **Safe external data handling**: validate and sanitize data from Mendix props before rendering or passing to DOM; guard against prototype pollution when merging objects
163+
- **No `eval()` or `new Function()`** with dynamic input
164+
- **Third-party scripts**: dynamic `<script>` injection must be reviewed for XSS risk
165+
- **URL handling**: verify `href`/`src` values derived from user input are validated (guard against `javascript:` and `data:` URIs)
166+
- **Event handlers**: avoid attaching global `window`/`document` listeners without cleanup — can leak references and be exploited
167+
168+
### Accessibility
169+
170+
Follow WCAG 2.2 AA. Prefer semantic HTML over ARIA — only add ARIA when native elements don't convey the right semantics.
171+
172+
- **Semantic elements**: use `<button>` for actions, `<a>` for navigation, `<dialog>` for modals — not `<div onClick>`
173+
- **Keyboard navigation**: all interactive elements reachable and operable by keyboard; focus order is logical
174+
- Arrow keys for menu/list navigation
175+
- Enter/Space to activate
176+
- Escape to dismiss floating elements
177+
- Roving `tabIndex` pattern for lists/menus (`tabIndex=0` on active item, `-1` on others)
178+
- **ARIA labels**: interactive elements without visible text need `aria-label` or `aria-labelledby`; dynamic regions need `aria-live` where appropriate
179+
- **Focus management**: dialogs/modals must trap focus on open and restore it on close; use `FloatingFocusManager` from Floating UI for popovers/menus
180+
- **Images**: decorative images use `alt=""`; informative images have descriptive `alt` text
181+
- **Colour contrast**: minimum 4.5:1 for normal text, 3:1 for large text and UI components (AA)
182+
- **No content conveyed by colour alone**: pair colour cues with text or icons
183+
184+
## Heuristics
185+
186+
| Situation | Comment |
187+
| ---------------------------------------------- | ------------------------------------------------------------------------------ |
188+
| Code/XML changed, no version bump or CHANGELOG | "Please bump semver and add changelog (`pnpm -w changelog`)." |
189+
| Feature/fix without tests | "Please add unit tests in `src/components/__tests__/` and consider E2E tests." |
190+
| XML changed, TS props not updated | "XML props changed but TS types/usage aren't aligned." |
191+
| Async effect sets state without guard | Suggest the `active` flag pattern above |
192+
| `index` used as list `key` | Request a stable unique key |
193+
| MobX mutation outside `action` | Suggest `runInAction` or `action` wrapper |
194+
| `dangerouslySetInnerHTML` without sanitization | Flag as high severity; require DOMPurify or equivalent |
195+
| Hardcoded secret, token, or credential | Flag as critical; must be removed and rotated |
196+
| `javascript:` or `data:` URI from user input | Flag as XSS risk; require validation before use |
197+
| Interactive element is a `<div>` with onClick | Replace with `<button>` or `<a>`; add keyboard handler if div must be kept |
198+
| Missing `alt` on `<img>` | Add descriptive `alt` or `alt=""` for decorative images |
199+
| No `aria-label` on icon-only button | Add `aria-label` describing the action |
200+
| Colour used as sole differentiator | Pair with text or icon; check contrast ratio meets 4.5:1 (3:1 for large text) |
201+
202+
## Scope
203+
204+
### What to review
205+
206+
| Path pattern | What to check |
207+
| -------------------------------------------------------------- | ---------------------------------------------------------------------------------- |
208+
| `packages/pluggableWidgets/*/src/**/*.{ts,tsx}` | Widget logic, React hooks, MobX, Mendix data API usage |
209+
| `packages/pluggableWidgets/*/*.xml` | Widget manifest: property keys (lowerCamelCase), unique ID, XML ↔ TS alignment |
210+
| `packages/pluggableWidgets/*/**/*.scss` | Styling: BEM naming, Atlas UI classes, no `!important`, no inline styles |
211+
| `packages/pluggableWidgets/*/src/**/__tests__/*.spec.{ts,tsx}` | Unit test coverage, builder usage, RTL patterns |
212+
| `packages/pluggableWidgets/*/e2e/*.spec.js` | E2E structure, selectors, afterEach logout, no hardcoded waits |
213+
| `packages/pluggableWidgets/*/package.json` | Semver bump present when runtime/XML/behavior changed |
214+
| `packages/pluggableWidgets/*/CHANGELOG.md` | Keep a Changelog entry present when version bumped |
215+
| `packages/pluggableWidgets/*/*.editorConfig.ts` | Studio Pro design-time config aligns with XML properties |
216+
| `packages/pluggableWidgets/*/*.editorPreview.tsx` | Preview component renders without crashing; no production-only imports |
217+
| `packages/shared/*/src/**/*.{ts,tsx}` | Shared utility changes — check for breaking API changes affecting widget consumers |
218+
| `packages/modules/*/src/**/*.{ts,tsx}` | Module-level logic and Mendix integration patterns |
219+
| `automation/**/*.{ts,mjs,cjs}` | Build/test automation scripts — no destructive ops, no hardcoded paths |
220+
| `.github/workflows/*.yml` | See workflow rules below |
221+
| `.claude/skills/**` | See skill rules below |
222+
223+
### What to ignore
224+
225+
- `dist/**` — build output, never review
226+
- `pnpm-lock.yaml` — lockfile-only changes need no review unless paired with `package.json` changes
227+
- `**/.turbo/**`, `**/node_modules/**` — generated/cached artifacts
228+
- `**/test-results/**`, `**/results/**` — test output directories
229+
- `dist/tmp/**` — intermediate build artifacts
230+
- `**/__snapshots__/**` — auto-generated snapshots (flag only if snapshot was deleted without test change)
231+
- `*.mpk` — compiled Mendix packages
232+
233+
### Multi-package PRs
234+
235+
When a PR touches multiple packages, validate each changed package separately:
236+
237+
- Versioning and CHANGELOG per package
238+
- XML ↔ TS alignment per widget
239+
- Test coverage per changed component
240+
241+
### GitHub Actions workflows (`.github/workflows/**`)
242+
243+
- All action references must be SHA-pinned with a version comment: `uses: actions/foo@<sha> # vX.Y`
244+
- Secrets must be read from `${{ secrets.* }}` — never hardcoded values
245+
- Permissions must be minimal: only declare what the job actually needs
246+
- Jobs that assume AWS/cloud roles must have `id-token: write` and use OIDC (`aws-actions/configure-aws-credentials`)
247+
- Jobs running on `pull_request` events should guard against fork PRs: check `head.repo.full_name` or `github.repository`
248+
- Jobs triggerable by comments (`issue_comment`) must restrict by `author_association` to prevent external contributors from triggering privileged workflows
249+
- Every long-running job should have `timeout-minutes` set
250+
- Shared `concurrency` groups with `cancel-in-progress: true` will cancel in-flight jobs — use per-job concurrency for interactive workflows
251+
252+
### Claude Code skills (`.claude/skills/**`)
253+
254+
- Frontmatter (`name`, `description`) must be present and accurate
255+
- Instructions should be actionable and specific — vague guidance leads to inconsistent reviews
256+
- Code examples must match the patterns actually used in this repo (check against `src/**` if unsure)
257+
- New sections should include a "What to flag" list of concrete anti-patterns
258+
259+
## Output format
260+
261+
- Inline comments for specific issues — reference file and line, include short code snippet
262+
- **Always post a summary comment** — even if no issues are found. Use this template:
263+
264+
**Clean PR:**
265+
266+
```
267+
## AI Code Review — no issues found
268+
269+
Reviewed files: <list changed files that were in scope>
270+
Skipped (out of scope): <list ignored files, e.g. dist/, lockfile>
271+
272+
No issues found. ✓
273+
```
274+
275+
**PR with issues:**
276+
277+
```
278+
## AI Code Review
279+
280+
Reviewed files: <list changed files that were in scope>
281+
Skipped (out of scope): <list ignored files>
282+
283+
### Summary
284+
<overall assessment — severity, what was flagged>
285+
```
286+
287+
- Be specific and actionable; avoid noise
288+
- The summary comment is mandatory — it is the confirmation that the review actually ran

‎.github/workflows/ClaudeReview.yml‎

Lines changed: 7 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -48,33 +48,17 @@ jobs:
4848
uses: anthropics/claude-code-action@fefa07e9c665b7320f08c3b525980457f22f58aa # v1
4949
with:
5050
use_bedrock: "true"
51-
model: ${{ env.CLAUDE_MODEL }}
5251
github_token: ${{ secrets.GITHUB_TOKEN }}
5352
prompt: |
5453
You are reviewing PR #${{ github.event.pull_request.number }} in the mendix/web-widgets monorepo.
5554
56-
IMPORTANT: Read `.github/copilot-instructions.md` for the complete review guidelines. Follow those instructions precisely.
55+
Read these files for full context and review guidelines:
56+
- `AGENTS.md` — repo conventions, commands, and constraints
57+
- `.claude/skills/code-review/SKILL.md` — complete review checklist and heuristics
5758
58-
Focus your review on:
59-
1. Mendix pluggable widget conventions (XML/TSX alignment, data API usage, canExecute checks)
60-
2. React hooks correctness (dependencies, stale closures, async effect guards)
61-
3. MobX store patterns (makeObservable, action boundaries, React integration via useSubscribe)
62-
4. Versioning: if runtime/XML/behavior changes exist, require semver bump + CHANGELOG.md entry
63-
5. Test coverage for new features and bug fixes
64-
6. SCSS/styling following Atlas UI conventions (no inline styles, no overriding core Atlas classes)
65-
66-
Review scope:
67-
- Focus ONLY on changed files (the diff)
68-
- Ignore dist/, lockfile changes, and generated files
69-
- For Renovate/dependency-only PRs, check for breaking changes only
70-
71-
Output format:
72-
- Use inline comments for specific code issues with actionable suggestions
73-
- Include short code snippets when suggesting fixes
74-
- Be specific and reference files/lines
75-
- Post a brief summary comment with overall assessment
59+
Follow the skill guidelines precisely. Focus only on changed files in the diff; ignore dist/, lockfiles, and generated files.
7660
claude_args: >-
77-
--allowedTools "Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr comment:*),Bash(gh api:*),mcp__github_inline_comment__create_inline_comment,Read,Grep,Glob"
61+
--model ${{ env.CLAUDE_MODEL }} --allowedTools "Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr comment:*),Bash(gh api:*),mcp__github_inline_comment__create_inline_comment,Read,Grep,Glob"
7862
7963
interactive:
8064
name: Claude Interactive
@@ -90,7 +74,7 @@ jobs:
9074
runs-on: ubuntu-latest
9175
timeout-minutes: 15
9276
concurrency:
93-
group: claude-interactive-${{ github.event.issue.number }}
77+
group: claude-interactive-${{ github.event.issue.number || github.event.pull_request.number }}
9478
cancel-in-progress: false
9579
permissions:
9680
contents: read
@@ -114,8 +98,7 @@ jobs:
11498
uses: anthropics/claude-code-action@fefa07e9c665b7320f08c3b525980457f22f58aa # v1
11599
with:
116100
use_bedrock: "true"
117-
model: ${{ env.CLAUDE_MODEL }}
118101
github_token: ${{ secrets.GITHUB_TOKEN }}
119102
trigger_phrase: "@claude"
120103
claude_args: >-
121-
--allowedTools "Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr comment:*),Bash(gh api:*),mcp__github_inline_comment__create_inline_comment,Read,Grep,Glob"
104+
--model ${{ env.CLAUDE_MODEL }} --allowedTools "Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr comment:*),Bash(gh api:*),mcp__github_inline_comment__create_inline_comment,Read,Grep,Glob"

0 commit comments

Comments
 (0)