Skip to content

Commit 370dbb5

Browse files
leonardomendixleonardomendix
authored
[CRAFTING] Add AI code review with Claude Code Action and AWS Bedrock (#2199)
2 parents 83978c8 + 87e4863 commit 370dbb5

2 files changed

Lines changed: 457 additions & 0 deletions

File tree

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

Lines changed: 348 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,348 @@
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+
### Changelog (per changed package)
19+
20+
Version bumps happen in a separate dedicated PR — do not require or flag missing semver bumps.
21+
22+
If runtime code, public API, XML schema, or behavior changed:
23+
24+
- Require `CHANGELOG.md` entry (Keep a Changelog format)
25+
- Suggest: `pnpm -w changelog`
26+
27+
If refactor/docs/tests-only: changelog entry not required — confirm with author.
28+
29+
### Mendix-specific
30+
31+
`AGENTS.md` covers the core rules (canExecute, loading states, lowerCamelCase XML keys). Flag these additional review issues:
32+
33+
- XML changed but TS props not updated, or widget ID is not unique
34+
- `EditableValue` read without checking `.status` — can render stale/undefined data
35+
- `ActionValue.execute()` called without checking `.canExecute` first
36+
37+
### React
38+
39+
Flag these patterns — general React conventions are assumed known:
40+
41+
- Missing or wrong `useEffect`/`useMemo`/`useCallback` deps; stale closures
42+
- Async effect sets state without a cleanup guard:
43+
```ts
44+
useEffect(() => {
45+
let active = true;
46+
fetchData().then(data => {
47+
if (active) setState(data);
48+
});
49+
return () => {
50+
active = false;
51+
};
52+
}, [fetchData]);
53+
```
54+
- Array index used as list `key` — requires a stable unique key
55+
- Props spread onto DOM nodes (`<div {...props}>`) — strips unknown HTML attributes
56+
57+
### MobX
58+
59+
- `makeAutoObservable` or `makeObservable` missing from store constructor
60+
- State mutation outside `action` — suggest `runInAction` or `action` wrapper
61+
- `computed` with side effects — must be pure
62+
- Missing `observer` HOC or `useSubscribe()` from `@mendix/widget-plugin-mobx-kit` on React components that read MobX state
63+
64+
### Styling
65+
66+
Conventions are in `docs/requirements/frontend-guidelines.md`. Flag only deviations:
67+
68+
- Inline styles used for static design
69+
- Core Atlas UI classes overridden
70+
- `!important` used
71+
- CSS class not prefixed with widget name
72+
73+
### Unit tests
74+
75+
Files live in `src/**/__tests__/*.spec.ts(x)` and run with Jest + RTL (enzyme-free).
76+
77+
**Structure**
78+
79+
- Use `describe`/`it` blocks; group related cases under a nested `describe`
80+
- Define a `defaultProps` constant and a factory render helper to avoid repetition:
81+
```ts
82+
const defaultProps: MyWidgetProps = { ... };
83+
const renderWidget = (props = defaultProps) => render(<MyWidget {...props} />);
84+
```
85+
86+
**Mendix data mocking — always use builders, never manual objects**
87+
88+
```ts
89+
import { EditableValueBuilder, ListValueBuilder, actionValue, obj } from "@mendix/widget-plugin-test-utils";
90+
91+
const value = new EditableValueBuilder<string>().withValue("hello").build();
92+
const readOnly = new EditableValueBuilder<string>().withValue("x").isReadOnly().build();
93+
const loading = new EditableValueBuilder<string>().isLoading().build();
94+
const list = new ListValueBuilder().withItems([obj("A"), obj("B")]).build();
95+
const action = actionValue(); // jest.fn() — assert with .execute toHaveBeenCalled()
96+
```
97+
98+
**What to cover**
99+
100+
- All Mendix data states: `Available`, `Loading`, `Unavailable`, `ReadOnly`
101+
- All prop/behavior branches (null checks, conditional renders, edge cases)
102+
- User interactions via `fireEvent` or `userEvent`
103+
- Verify `setValue` / `execute` calls: `expect(action.execute).toHaveBeenCalled()`
104+
- Accessibility assertions: `getByRole`, `getByLabelText`, ARIA attributes
105+
106+
**What to flag**
107+
108+
- Manual mock objects instead of builders — brittle and miss status edge cases
109+
- Enzyme patterns (`shallow`, `mount`, `instance()`) — must use RTL
110+
- Snapshot tests on dynamic content (dates, IDs, async state) — use specific assertions instead
111+
- Missing `afterEach` mock cleanup — causes test pollution
112+
- Missing `ResizeObserver` / `window.mx` global setup when widget requires it (add to `jest.setup.ts`)
113+
- Jest config not extending `@mendix/pluggable-widgets-tools/test-config/jest.config`
114+
115+
### E2E tests
116+
117+
Files live in `e2e/*.spec.js` and run with Playwright (Chromium). Config inherits from `@mendix/run-e2e/playwright.config.cjs`.
118+
119+
**Mandatory structure — every file must have this**
120+
121+
```js
122+
import { test, expect } from "@playwright/test";
123+
124+
test.afterEach("Cleanup session", async ({ page }) => {
125+
await page.evaluate(() => window.mx.session.logout());
126+
});
127+
128+
test.describe("WidgetName", () => {
129+
test.beforeEach(async ({ page }) => {
130+
await page.goto("/");
131+
await page.waitForLoadState("networkidle");
132+
});
133+
});
134+
```
135+
136+
**Selectors — in order of preference**
137+
138+
1. `.mx-name-*` — Mendix widget names (most stable): `page.locator(".mx-name-myWidget")`
139+
2. ARIA roles: `page.getByRole("button", { name: "Save" })`
140+
3. Widget CSS classes: `page.locator(".widget-badge-button-text")`
141+
4. Avoid brittle selectors: nth-child chains, deeply nested CSS, text-only locators
142+
143+
**Assertions**
144+
145+
```js
146+
await expect(page.locator(".mx-name-myWidget")).toBeVisible();
147+
await expect(page.locator(".badge")).toContainText("New");
148+
await expect(page.locator(".mx-name-myWidget")).toHaveScreenshot("myWidget-default.png");
149+
```
150+
151+
**Accessibility scanning (use for new interactive widgets)**
152+
153+
```js
154+
import AxeBuilder from "@axe-core/playwright";
155+
const results = await new AxeBuilder({ page }).analyze();
156+
expect(results.violations).toEqual([]);
157+
```
158+
159+
**What to flag**
160+
161+
- Missing `afterEach` session logout — will exceed Mendix's 5-session license limit in CI
162+
- `page.waitForTimeout()` / hardcoded `sleep` — replace with `waitForLoadState` or a Playwright locator assertion
163+
- Selectors that don't use `.mx-name-*` when a Mendix widget name is available
164+
- Screenshot baselines not committed — `toHaveScreenshot` requires a baseline PNG in the repo
165+
- Missing `page.waitForLoadState("networkidle")` in `beforeEach` — causes flaky tests
166+
- E2E file not following `WidgetName.spec.js` naming convention
167+
168+
### Security
169+
170+
- **No `dangerouslySetInnerHTML`** unless input is sanitized with a trusted library (e.g. DOMPurify); flag any unsanitized usage as high severity
171+
- **No secrets or tokens** hardcoded in source — API keys, client IDs, URLs with credentials
172+
- **Safe external data handling**: validate and sanitize data from Mendix props before rendering or passing to DOM; guard against prototype pollution when merging objects
173+
- **No `eval()` or `new Function()`** with dynamic input
174+
- **Third-party scripts**: dynamic `<script>` injection must be reviewed for XSS risk
175+
- **URL handling**: verify `href`/`src` values derived from user input are validated (guard against `javascript:` and `data:` URIs)
176+
- **Event handlers**: avoid attaching global `window`/`document` listeners without cleanup — can leak references and be exploited
177+
178+
### Accessibility
179+
180+
Full requirements are in `docs/requirements/frontend-guidelines.md`. Flag only deviations:
181+
182+
- Interactive element is a `<div>` or `<span>` with `onClick` — replace with `<button>` or `<a>`
183+
- Icon-only button missing `aria-label`
184+
- `<img>` missing `alt` attribute (use `alt=""` for decorative)
185+
- Dialog/modal does not trap focus or restore it on close — use `FloatingFocusManager`
186+
- Colour used as the sole differentiator — pair with text or icon
187+
188+
## Heuristics
189+
190+
| Situation | Severity | Comment |
191+
| ---------------------------------------------- | -------- | ------------------------------------------------------ |
192+
| Code/XML changed, no CHANGELOG entry | Medium | `pnpm -w changelog` — version bumps are a separate PR |
193+
| Feature/fix without tests | Medium | Add unit tests; consider E2E for interactive behaviour |
194+
| `dangerouslySetInnerHTML` without sanitization | High | Require DOMPurify or equivalent before merge |
195+
| Hardcoded secret, token, or credential | Critical | Must be removed and rotated immediately |
196+
| `javascript:` or `data:` URI from user input | High | Validate before use — XSS risk |
197+
198+
## Scope
199+
200+
### What to review
201+
202+
| Path pattern | What to check |
203+
| -------------------------------------------------------------- | ---------------------------------------------------------------------------------- |
204+
| `packages/pluggableWidgets/*/src/**/*.{ts,tsx}` | Widget logic, React hooks, MobX, Mendix data API usage |
205+
| `packages/pluggableWidgets/*/*.xml` | Widget manifest: property keys (lowerCamelCase), unique ID, XML ↔ TS alignment |
206+
| `packages/pluggableWidgets/*/**/*.scss` | Styling: BEM naming, Atlas UI classes, no `!important`, no inline styles |
207+
| `packages/pluggableWidgets/*/src/**/__tests__/*.spec.{ts,tsx}` | Unit test coverage, builder usage, RTL patterns |
208+
| `packages/pluggableWidgets/*/e2e/*.spec.js` | E2E structure, selectors, afterEach logout, no hardcoded waits |
209+
| `packages/pluggableWidgets/*/package.json` | Version bumps happen in a separate PR — do not flag missing bumps |
210+
| `packages/pluggableWidgets/*/CHANGELOG.md` | Keep a Changelog entry present when runtime/XML/behavior changed |
211+
| `packages/pluggableWidgets/*/*.editorConfig.ts` | Studio Pro design-time config aligns with XML properties |
212+
| `packages/pluggableWidgets/*/*.editorPreview.tsx` | Preview component renders without crashing; no production-only imports |
213+
| `packages/shared/*/src/**/*.{ts,tsx}` | Shared utility changes — check for breaking API changes affecting widget consumers |
214+
| `packages/modules/*/src/**/*.{ts,tsx}` | Module-level logic and Mendix integration patterns |
215+
| `automation/**/*.{ts,mjs,cjs}` | Build/test automation scripts — no destructive ops, no hardcoded paths |
216+
| `.github/workflows/*.yml` | See workflow rules below |
217+
| `.claude/skills/**` | See skill rules below |
218+
219+
### What to ignore
220+
221+
- `dist/**` — build output, never review
222+
- `pnpm-lock.yaml` — lockfile-only changes need no review unless paired with `package.json` changes
223+
- `**/.turbo/**`, `**/node_modules/**` — generated/cached artifacts
224+
- `**/test-results/**`, `**/results/**` — test output directories
225+
- `dist/tmp/**` — intermediate build artifacts
226+
- `**/__snapshots__/**` — auto-generated snapshots (flag only if snapshot was deleted without test change)
227+
- `*.mpk` — compiled Mendix packages
228+
229+
### Multi-package PRs
230+
231+
When a PR touches multiple packages, validate each changed package separately:
232+
233+
- CHANGELOG entry per package (version bumps are out of scope)
234+
- XML ↔ TS alignment per widget
235+
- Test coverage per changed component
236+
237+
### GitHub Actions workflows (`.github/workflows/**`)
238+
239+
- All action references must be SHA-pinned with a version comment: `uses: actions/foo@<sha> # vX.Y`
240+
- Secrets must be read from `${{ secrets.* }}` — never hardcoded values
241+
- Permissions must be minimal: only declare what the job actually needs
242+
- Jobs that assume AWS/cloud roles must have `id-token: write` and use OIDC (`aws-actions/configure-aws-credentials`)
243+
- Jobs running on `pull_request` events should guard against fork PRs: check `head.repo.full_name` or `github.repository`
244+
- Jobs triggerable by comments (`issue_comment`) must restrict by `author_association` to prevent external contributors from triggering privileged workflows
245+
- Every long-running job should have `timeout-minutes` set
246+
- Shared `concurrency` groups with `cancel-in-progress: true` will cancel in-flight jobs — use per-job concurrency for interactive workflows
247+
248+
### Claude Code skills (`.claude/skills/**`)
249+
250+
- Frontmatter (`name`, `description`) must be present and accurate
251+
- Instructions should be actionable and specific — vague guidance leads to inconsistent reviews
252+
- Code examples must match the patterns actually used in this repo (check against `src/**` if unsure)
253+
- New sections should include a "What to flag" list of concrete anti-patterns
254+
255+
## Output format
256+
257+
**Determine the execution context first** by checking whether the `CI` environment variable is set (`echo $CI`):
258+
259+
- **CI environment** (`CI=true`): post the review as a PR comment using `gh`, following the template below.
260+
- **Local environment** (`CI` unset or empty): print the review directly to the terminal in the same format. Do NOT post any `gh` comment.
261+
262+
Use inline comments for issues that reference a specific line; use the summary comment/output for file-level or cross-cutting issues.
263+
264+
### Summary comment template
265+
266+
**Verdict line** — always the first line, one of:
267+
268+
- `✅ Approved — no issues found`
269+
- `⚠️ Approved with suggestions — low-severity items only, safe to merge`
270+
- `🔶 Changes requested — one or more medium-severity items must be addressed`
271+
- `🚨 Blocked — high-severity issue (security, data loss, broken API) must be fixed`
272+
273+
**Post the comment using exactly this structure** (GitHub Markdown — render as-is, do not wrap in a code fence):
274+
275+
The comment must start with:
276+
277+
```
278+
## AI Code Review
279+
```
280+
281+
Then a blockquote verdict on the next line:
282+
283+
```
284+
> ✅ Approved — no issues found
285+
```
286+
287+
(replace with the appropriate verdict emoji and text)
288+
289+
Then a horizontal rule `---`, then a `### What was reviewed` section with a Markdown table:
290+
291+
```
292+
### What was reviewed
293+
294+
| File | Change |
295+
| --- | --- |
296+
| `path/to/file.ts` | Brief description of what changed |
297+
| `path/to/other.yml` | Brief description |
298+
299+
Skipped (out of scope): `dist/`, `pnpm-lock.yaml`
300+
```
301+
302+
Then `---`, then a `### Findings` section (omit entirely on a clean PR) where each finding is a level-4 heading:
303+
304+
```
305+
### Findings
306+
307+
#### 🚨 High — <short title>
308+
309+
**File:** `path/to/file.ts` line 42
310+
**Problem:** What is wrong and why it matters.
311+
**Fix:**
312+
\`\`\`ts
313+
// suggested fix snippet
314+
\`\`\`
315+
316+
---
317+
318+
#### 🔶 Medium — <short title>
319+
320+
**File:** `path/to/file.ts` line 10
321+
**Problem:** What is wrong and why it matters.
322+
**Fix:** What to do (prose or snippet).
323+
324+
---
325+
326+
#### ⚠️ Low — <short title>
327+
328+
**File:** `path/to/file.ts` line 5
329+
**Note:** What to consider, not blocking.
330+
```
331+
332+
Then `---`, then a `### Positives` section as a bullet list:
333+
334+
```
335+
### Positives
336+
337+
- Specific thing done well (not generic praise)
338+
- Another concrete positive
339+
```
340+
341+
### Rules
342+
343+
- **The summary comment is mandatory** — even on a clean PR. It proves the review ran.
344+
- Omit the **Findings** section entirely on a clean PR; do not write "no findings".
345+
- Each finding must include: severity emoji, a short title, the file + line, the problem, and a concrete fix.
346+
- Keep **Positives** specific — name what was done right (e.g. "SHAs are pinned with version comments"), not generic ("good job").
347+
- One finding per `####` block — do not bundle multiple issues.
348+
- Be specific and actionable; avoid noise.

0 commit comments

Comments
 (0)