|
| 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 | +- **Review**: `src/**`, `*.xml`, `*.scss`, `package.json`, `CHANGELOG.md`, build/test configs |
| 205 | +- **Ignore**: `dist/**`, lockfile-only changes, generated files |
| 206 | + |
| 207 | +## Output format |
| 208 | + |
| 209 | +- Inline comments for specific issues — reference file and line, include short code snippet |
| 210 | +- Summary comment with overall assessment at the end |
| 211 | +- Be specific and actionable; avoid noise |
0 commit comments