Skip to content

Commit d39b346

Browse files
labkey-jeckelslabkey-jeckels
committed
Merge branch 'develop' into fb_intellijMcp
2 parents 8c58d26 + 495fa6b commit d39b346

13 files changed

Lines changed: 357 additions & 48 deletions

File tree

‎.agents/review-checklists/react/code-quality.md‎

Lines changed: 262 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,268 @@ const MyComponent: FC<MyComponentProps> = ({ report, tab }) => {
4343
};
4444
```
4545

46+
## React components and hooks should have unit tests
47+
48+
**Urgency:** suggestion
49+
50+
### Category
51+
52+
Maintainability
53+
54+
### Confidence Threshold
55+
56+
Flag new or significantly modified components/hooks lacking Jest tests when they contain logic (state, effects, event handlers, conditional rendering). Simple presentational components rendering static JSX may defer testing. Existing untested code not modified in the PR should not be flagged.
57+
58+
### Exceptions / False Positives
59+
60+
- Do not flag demo-only, internal layout wrappers, or placeholder components.
61+
- Do not flag when tests exist in a different location (e.g., shared test suite) if verifiable.
62+
- Do not flag existing untested code outside the PR scope.
63+
64+
### Description
65+
66+
Components with state, effects, event handlers, or conditional logic need Jest tests. Test what should always be covered: `useState`/`useReducer`, event handlers, conditional rendering, custom hooks with logic, permission-gated rendering. Layout-only components are optional.
67+
68+
### Anti-patterns to Flag
69+
70+
```tsx
71+
// ❌ Stateful component with events but no tests
72+
const Counter: FC = () => {
73+
const [count, setCount] = useState(0);
74+
return (
75+
<div>
76+
<p>Count: {count}</p>
77+
<button onClick={() => setCount(count + 1)}>Increment</button>
78+
</div>
79+
);
80+
};
81+
82+
// ❌ Custom hook with async logic but no tests
83+
const useItemData = (id: string) => {
84+
const [data, setData] = useState(null);
85+
useEffect(() => {
86+
// Error handling omitted for brevity
87+
async function load() {
88+
const result = await loadItemById(id);
89+
setData(result);
90+
}
91+
load();
92+
}, [id]);
93+
return data;
94+
};
95+
```
96+
97+
### Suggested Fix
98+
99+
Create `Component.test.tsx` with meaningful assertions per [`jest/business-logic.md`](../jest/business-logic.md):
100+
101+
```tsx
102+
test('increments count when button clicked', async () => {
103+
render(<Counter />);
104+
await userEvent.click(screen.getByRole('button', { name: /increment/i }));
105+
expect(screen.getByText('Count: 1')).toBeInTheDocument();
106+
});
107+
```
108+
109+
### How to Detect
110+
111+
1. Check for `.test.tsx`/`.test.ts` file corresponding to modified component.
112+
2. If missing, flag if component has: hooks, event handlers, or conditional logic.
113+
3. If tests exist, verify they assert on behavior (not just render existence).
114+
115+
## Large JSX/TSX blocks should be extracted into separate components
116+
117+
**Urgency:** suggestion
118+
119+
### Category
120+
121+
Maintainability
122+
123+
### Confidence Threshold
124+
125+
Flag JSX return statements >25 lines or those with multiple conditional branches (3+ `{condition && <...>}` or ternary operators). Simple single-element returns or small layout wrappers are exempt.
126+
127+
### Exceptions / False Positives
128+
129+
- Do not flag simple, single-element returns or small layout wrappers.
130+
131+
### Description
132+
133+
Large JSX blocks reduce readability, increase cognitive load, and make testing harder. Red flags: multiple conditional rendering branches, repeated patterns (like tab buttons with similar structure), deeply nested element hierarchies, and components managing multiple independent concerns.
134+
135+
### Anti-patterns to Flag
136+
137+
```tsx
138+
// ❌ Return block with multiple conditional branches and repeated patterns
139+
const DesignerPanel: FC<Props> = ({ data, showWarnings }) => {
140+
const [rightTab, setRightTab] = useState<'properties' | 'warnings'>('properties');
141+
142+
return (
143+
<div className="designer">
144+
{showWarnings && (
145+
<div className="tabs" role="tablist">
146+
<button
147+
role="tab"
148+
aria-selected={rightTab === 'properties'}
149+
onClick={() => setRightTab('properties')}
150+
>
151+
Properties
152+
</button>
153+
<button
154+
role="tab"
155+
aria-selected={rightTab === 'warnings'}
156+
onClick={() => setRightTab('warnings')}
157+
>
158+
Warnings
159+
</button>
160+
</div>
161+
)}
162+
{(!showWarnings || rightTab === 'properties') && (
163+
<div role="tabpanel">
164+
<PropertiesPanel data={data} />
165+
</div>
166+
)}
167+
{showWarnings && rightTab === 'warnings' && (
168+
<div role="tabpanel">
169+
<WarningsPanel data={data} />
170+
</div>
171+
)}
172+
</div>
173+
);
174+
};
175+
```
176+
177+
**Red flags in this example:**
178+
- 3 separate conditional render blocks
179+
- Repeated tab button structure with duplicated accessibility attrs
180+
- Multiple independent concerns (tabs, properties, warnings)
181+
- Mixed conditional logic (both &&-chaining and ternaries)
182+
183+
### Suggested Fix
184+
185+
Extract the tab control into a reusable component:
186+
187+
```tsx
188+
interface TabPanelProps {
189+
active: 'properties' | 'warnings';
190+
onChange: (tab: 'properties' | 'warnings') => void;
191+
}
192+
193+
const TabPanel: FC<TabPanelProps> = ({ active, onChange }) => {
194+
const handlePropertiesClick = useCallback(() => onChange('properties'), [onChange]);
195+
const handleWarningsClick = useCallback(() => onChange('warnings'), [onChange]);
196+
197+
return (
198+
<div className="tabs" role="tablist">
199+
<button
200+
role="tab"
201+
aria-selected={active === 'properties'}
202+
onClick={handlePropertiesClick}
203+
>
204+
Properties
205+
</button>
206+
<button
207+
role="tab"
208+
aria-selected={active === 'warnings'}
209+
onClick={handleWarningsClick}
210+
>
211+
Warnings
212+
</button>
213+
</div>
214+
);
215+
};
216+
TabPanel.displayName = 'TabPanel';
217+
218+
const DesignerPanel: FC<Props> = ({ data, showWarnings }) => {
219+
const [rightTab, setRightTab] = useState<'properties' | 'warnings'>('properties');
220+
221+
return (
222+
<div className="designer">
223+
{showWarnings && <TabPanel active={rightTab} onChange={setRightTab} />}
224+
{(!showWarnings || rightTab === 'properties') && <PropertiesPanel data={data} />}
225+
{showWarnings && rightTab === 'warnings' && <WarningsPanel data={data} />}
226+
</div>
227+
);
228+
};
229+
DesignerPanel.displayName = 'DesignerPanel';
230+
```
231+
232+
### How to Detect
233+
234+
1. **Multiple conditional branches:** Count `{condition && <...>}` and ternary operators. 3+ suggests extraction.
235+
2. **Repeated patterns:** Similar button/input groups with duplicated props (e.g., `role="tab"`, `aria-selected`, `onClick`) → extract into component.
236+
3. **Multiple state sections:** Component with separate state for tabs/panels (e.g., `const [tab, setTab]` and `const [panel, setPanel]`) often signals multiple concerns.
237+
4. **Line count + nesting:** Return >25 lines OR deeply nested (3+ levels) conditional JSX → consider extraction.
238+
239+
## React components and hooks should have unit tests
240+
241+
**Urgency:** suggestion
242+
243+
### Category
244+
245+
Maintainability
246+
247+
### Confidence Threshold
248+
249+
Flag new or significantly modified components/hooks lacking Jest tests when they contain logic (state, effects, event handlers, conditional rendering). Simple presentational components rendering static JSX may defer testing. Existing untested code not modified in the PR should not be flagged.
250+
251+
### Exceptions / False Positives
252+
253+
- Do not flag demo-only, internal layout wrappers, or placeholder components.
254+
- Do not flag when tests exist in a different location (e.g., shared test suite) if verifiable.
255+
- Do not flag existing untested code outside the PR scope.
256+
257+
### Description
258+
259+
Components with state, effects, event handlers, or conditional logic need Jest tests. Test what should always be covered: `useState`/`useReducer`, event handlers, conditional rendering, custom hooks with logic, permission-gated rendering. Layout-only components are optional.
260+
261+
### Anti-patterns to Flag
262+
263+
```tsx
264+
// ❌ Stateful component with events but no tests
265+
const Counter: FC = () => {
266+
const [count, setCount] = useState(0);
267+
return (
268+
<div>
269+
<p>Count: {count}</p>
270+
<button onClick={() => setCount(count + 1)}>Increment</button>
271+
</div>
272+
);
273+
};
274+
275+
// ❌ Custom hook with async logic but no tests
276+
const useItemData = (id: string) => {
277+
const [data, setData] = useState(null);
278+
useEffect(() => {
279+
// Error handling omitted for brevity
280+
async function load() {
281+
const result = await loadItemById(id);
282+
setData(result);
283+
}
284+
load();
285+
}, [id]);
286+
return data;
287+
};
288+
```
289+
290+
### Suggested Fix
291+
292+
Create `Component.test.tsx` with meaningful assertions per [`jest/business-logic.md`](../jest/business-logic.md):
293+
294+
```tsx
295+
test('increments count when button clicked', async () => {
296+
render(<Counter />);
297+
await userEvent.click(screen.getByRole('button', { name: /increment/i }));
298+
expect(screen.getByText('Count: 1')).toBeInTheDocument();
299+
});
300+
```
301+
302+
### How to Detect
303+
304+
1. Check for `.test.tsx`/`.test.ts` file corresponding to modified component.
305+
2. If missing, flag if component has: hooks, event handlers, or conditional logic.
306+
3. If tests exist, verify they assert on behavior (not just render existence).
307+
46308
## Optional props that are always passed should be required
47309

48310
**Urgency:** suggestion

‎.agents/review-checklists/react/performance.md‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,56 @@
22

33
> **Prerequisite:** Review and apply the common guidelines in [`common.md`](../common.md) before using this checklist.
44
5+
## Event handlers should be memoized with `useCallback`
6+
7+
**Urgency:** urgent
8+
9+
### Category
10+
11+
Performance
12+
13+
### Confidence Threshold
14+
15+
Flag any event handler prop (`onClick`, `onChange`, `onSubmit`, etc.) that is an inline arrow function or a named function in the component body without `useCallback`. Do not flag handlers defined at module scope — they are already stable references.
16+
17+
### Exceptions / False Positives
18+
19+
- Do not flag event handlers defined at module scope.
20+
- Do not suggest `useCallback` for handlers with no component-scope dependencies — hoisting to module scope is preferable (a true constant reference with no Hook overhead).
21+
22+
### Description
23+
24+
All event handlers in a component body should be wrapped with `useCallback`, including those on native DOM elements. The overhead is negligible; omitting it has caused real performance issues and creates audit work whenever a future refactor forwards the handler to a memoized child.
25+
26+
Watch for the factory anti-pattern: `useCallback` wrapping a function that itself returns a function. The outer reference is stable, but invoking it still produces a new function reference on every render.
27+
28+
### Anti-patterns to Flag
29+
30+
```tsx
31+
// ❌ Inline arrow — new reference every render
32+
<button onClick={() => handleDelete(item.id)}>Delete</button>
33+
34+
// ❌ Factory pattern — getHandler(id) returns a new fn each render despite useCallback
35+
const getHandler = useCallback((id: string) => () => handleClick(id), [handleClick]);
36+
<Item onClick={getHandler(item.id)} />
37+
```
38+
39+
### Suggested Fix
40+
41+
```tsx
42+
const handleDelete = useCallback(() => {
43+
doDelete(item.id);
44+
}, [doDelete, item.id]);
45+
<button onClick={handleDelete}>Delete</button>
46+
```
47+
48+
### How to Detect
49+
50+
1. Search for `onClick={`, `onChange={`, etc. where the value is an inline arrow or an identifier not assigned via `useCallback(...)`.
51+
2. For existing `useCallback` calls, check if the wrapped function returns another function — if so, verify that returned function is not being passed directly as a prop.
52+
53+
---
54+
555
## Inline object/array literals in JSX props
656

757
**Urgency:** urgent

‎CLAUDE.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,14 @@ We keep local copies of the `@labkey` packages in the `clientAPIs/` directory. Y
8383

8484
The local copies of the packages may contain changes related to the current branches in any of the modules that have an NPM build. For example there may be changes to the `@labkey/components` package that affect the package in the `server/modules/platform/core` module. These packages are not required to be present to build the project, so they may not be available. If they are not present, you can assume that there are no changes in those packages relevant to the current branch. If you suspect an issue is with one of the packages, but it is not present you may prompt the user to check out a local copy of the relevant package.
8585

86+
### React Conventions
87+
- We are using React 18
88+
- **Component typing:** Use `FC<Props>` (or `FC` for no-prop components) for proper components used as `<Component />`. Use `ReactNode` as the return type for render helpers — functions that return JSX but are not used as components (typically named with a lowercase letter or a `render` prefix). Do **not** annotate with `JSX.Element` or `ReactElement`.
89+
- **React type imports:** Always import React types directly from `react` rather than accessing them via the `React.*` namespace. For example: `import { FC, ReactNode } from 'react'`, never `React.FC` or `React.ReactNode`.
90+
- **displayName:** Set `ComponentName.displayName = 'ComponentName'` immediately after each `React.FC` definition so it appears correctly in React DevTools and error boundaries.
91+
- **Props interfaces:** Declare a separate named `interface ComponentNameProps` for any component with two or more props. Do not inline the type in the `FC<>` generic.
92+
- **Event handlers:** Wrap all event handlers defined in a component body with `useCallback`, including those passed to native DOM elements. Do not use inline arrow functions in JSX (e.g., `onClick={() => doX()}`); extract and memoize them instead. Be aware that `useCallback` on a factory function (one that itself returns a new function) does not memoize the result — each invocation still produces a new reference.
93+
8694
### Distributions
8795

8896
The `distributions/` directory defines 60+ distribution configurations that select which modules to package. Distributions inherit from each other (most inherit from `:distributions:base`). Distribution directory names must not collide with module names.

‎build.gradle‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ plugins {
1616
id "com.jfrog.artifactory" version "${artifactoryPluginVersion}" apply false
1717
id "com.github.node-gradle.node" version "${gradleNodePluginVersion}" apply false
1818
id "org.owasp.dependencycheck" version "${owaspDependencyCheckPluginVersion}" apply false
19-
// id "com.github.ben-manes.versions" version "0.39.0"
19+
id "com.github.ben-manes.versions" version "0.54.0"
2020
id "org.labkey.build.multiGit"
2121
}
2222

@@ -48,7 +48,7 @@ allprojects {
4848
analyzers.ossIndex.enabled = false
4949
}
5050
formats = ['HTML', 'JUNIT']
51-
skipConfigurations = ['dedupe', 'gwtCompileClasspath', 'gwtRuntimeClasspath', 'developmentOnly']
51+
skipConfigurations = ['dedupe', 'developmentOnly']
5252
skipProjects = [':server:testAutomation']
5353

5454
nvd {
@@ -379,9 +379,6 @@ allprojects {
379379
force "org.springframework:spring-messaging:${springVersion}"
380380
force "org.springframework:spring-webflux:${springVersion}"
381381

382-
// spring-ai dependency. Force to mitigate a CVE.
383-
force "io.modelcontextprotocol.sdk:mcp:${modelContextProtocolVersion}"
384-
385382
// Force consistency between pipeline's ActiveMQ and cloud's jClouds dependencies
386383
force "javax.annotation:javax.annotation-api:${javaxAnnotationVersion}"
387384

@@ -589,7 +586,7 @@ project.tasks.register('ijConfigure') {
589586
task.dependsOn(project.tasks.ijRunConfigurationsSetup)
590587
}
591588

592-
if (project.hasProperty('artifactory_contextUrl') && project.hasProperty('artifactory_user') && project.hasProperty('artifactory_password'))
589+
if (BuildUtils.hasArtifactoryProperties(project as Project))
593590
{
594591
project.tasks.register('purgeNpmAlphaVersions', PurgeNpmAlphaVersions) {
595592
group = GroupNames.NPM_RUN

0 commit comments

Comments
 (0)