AI front end code review guidelines and claude skills by labkey-martyp · Pull Request #1292 · LabKey/server

labkey-martyp · 2026-02-23T13:47:35Z

Rationale

This is a starting point for frontend ai code review guidelines. There are two code review checklists and claude skills, /code-review-react and /code-review-jest. The claude skills have two modes,

Review only staged changes (changes not committed yet). ex. /code-review-react <directory or file path>
Review full files or directories. ex /code-review-jest --full <directory or file path>

See .agents/common.md for review priority and format.
See the "Required output" section of the .claude/skills/<skill name>/skill.md for the expected output. Note: If there are more than 10 "suggested" changes (non-urgent), only the first 10 will be shown in each run.
See the files in .agents/review-checklists/<react or jest>/*.md for the code review checks.

It is recommended to run the code reviews multiple times until all the feedback is addressed.

These guidelines can be used by other ai agents as well, just point them to the directory.

Changes

.agents/review-checklists/jest code review checks
.agents/review-checklists/react code review checks
.agents/review-checkslists/common.md general code review guidelines
.claude/skill/<skill name>/skill.md define the claude skill using these guidelines

labkey-jeckels

I don't have React or Jest changes handy to test this out, but the overall structure seems reasonable.

Others can better comment on whether these are the right items to highlight, but this might be best evaluated through actual testing.

I can see pulling some of this guidance into more general areas in the future. For example, the rules for code comments should be applicable to any language, not just React/TypeScript/JavaScript.

cnathe

I left some comments but I agree that this will need to be treated as a working document as we use it and until we start using it, it is hard to know exactly what we all want here. Looks like a great starting point.

cnathe · 2026-03-02T15:49:31Z

.claude/skills/code-review-jest/skill.md

@@ -0,0 +1,123 @@
+---
+name: code-review-jest
+description: "Trigger when the user requests a review of Jest test files (e.g., `.test.tsx`, `.spec.tsx`). Supports --full flag for complete file/directory review; defaults to staged changes when a path is provided."


we can remove the .spec.tsx part right? those are dead tests that we need to convert. Instead maybe make the files target .test.tsx and .test.ts?

I agree, if anything we should probably have another skill for converting enzyme tests to RTL, but I also don't think it's high priority enough require it go in this PR.

cnathe · 2026-03-02T15:49:55Z

.claude/skills/code-review-jest/skill.md

+# Jest Test Code Review
+
+## Intent
+Use this skill whenever the user asks to review Jest test code (especially `.test.tsx`, `.spec.tsx`, `.test.ts`, or `.spec.ts` files). Support three review modes:


same comment here about spec files

cnathe · 2026-03-02T16:10:32Z

.claude/skills/code-review-react/skill.md

+4. Compose the review section per the template below. Group findings by **Urgency** section first (urgent issues, then suggestions). Within each section, order findings by the checklist primary category priority: **Correctness**, then **Maintainability**, then **Style**.
+
+## Required output
+When invoked, the response must exactly follow one of the two templates:


so much of the text / instructions above this line in this file are so close to a duplicate of the code-review-react/skill.md file. This likely makes sense as there might not be a good way to share that part of these instructions. I'm just a little worried about how we are goign to keep these things up to date and in sync.

cnathe · 2026-03-02T16:17:27Z

.agents/review-checklists/jest/code-quality.md

+
+### Confidence Threshold
+
+Flag when the symbol is clearly unused in the file or when a mock setup is not consumed by behavior under test. If a mock or import may be used implicitly (module mocking side effects, global setup conventions), verify before commenting.


what does "the symbol" mean in this sentence? do you mean variable?

cnathe · 2026-03-02T16:19:16Z

.agents/review-checklists/jest/code-quality.md

+6. **Unused parameters in callbacks must be prefixed with `_`.** If a callback parameter (e.g., in `.mockImplementation((unusedArg) => ...)`) is required for positional reasons but not used, prefix it with `_` to signal intent (e.g., `_unusedArg`).
+7. **Unused `render` results must not be destructured.** If `render(<Component />)` is called and the return value is not used, do not destructure it. Write `render(<Component />);` instead of `const { container } = render(<Component />);` when `container` is never referenced.
+
+### Examples


this is great. I'd love to not have to manually check as a code reviewer if the import or variable is used!

cnathe · 2026-03-02T16:21:22Z

.agents/review-checklists/jest/performance.md

+
+> **Prerequisite:** Review and apply the common guidelines in [`common.md`](../common.md) before using this checklist.
+
+## No redundant or near-duplicate tests


I'm curious what AI would consider a near-duplicate test. I think using near duplicate tests is exactly how test cases often look when you are testing interactions of a sequence of parameters or properties (i.e. adjust prop1, then prop2, then prop3, etc. but the test is nearly the same).

So, in those cases we should be using something like jest.each, where we have one shared test body, but you parametrize the test with an array of args. We don't do this a lot right now, but we should for sure be doing it more.

cnathe · 2026-03-02T16:23:44Z

.agents/review-checklists/react/business-logic.md

+
+// ✅ Fallback for optional props
+const [text, setText] = useState(props.initialText ?? '');
+```


this is great. these are always tough things to catch when manually scrolling through a large PR code change.

cnathe · 2026-03-02T16:25:45Z

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

+};
+```
+
+## Optional props that are always passed should be required


this one has been debated a few times before by the Frontend group and I think people fall on different camps here. Seems fine to include as long as we are making an effort to be sure to update / adjust this set of rules as we work through usages.

I don't think this has been debated. I'm the one who has brought this to the group multiple times, basically anytime I've encountered an issue caused by this, or when I've done large refactors and discovered multiple type related issues, and I don't recall a time that I have gotten push back on this. The upside is that by following this rule your types are more correct, and it eliminates a whole class of bug, the downside is that it's slightly more verbose.

cnathe · 2026-03-02T16:27:48Z

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

+- **CSS Modules:** When using CSS Modules (`.module.css` / `.module.scss`), camelCase class names are standard since they become JS property names (e.g., `styles.searchForm`, `styles.activeItem`). Do not flag these as BEM violations.
+- **Existing non-BEM code:** Class names in unchanged code that predate BEM adoption should not be flagged. Only apply BEM conventions to new or actively modified class names in the PR.
+
+## No orphaned or unused CSS/SCSS rules


this would be great if AI can help with this one! this is very tedious to track down manually

labkey-alan

I think overall these rules are fine. I did leave some feedback that you should review before merging. If you have follow up questions, or want to have a discussion, let me know.

My biggest issue with these rules at the moment is that I don't really think this is the appropriate place for them, as this keeps them scoped to LabKey server and modules defined within it, but it excludes their availability to our packages e.g. labkey-ui-components, labkey-ui-premium, which is where the majority of our react and jest code lives. I also don't really want a solution that involves duplicating these rules across every repo that needs them. That said, I think this is an acceptable place to put them for now, as it allows people to experiment with them today, albeit in a limited scope. We can always move these rules to a more ideal location later.

labkey-alan · 2026-03-02T17:16:26Z

.agents/review-checklists/jest/business-logic.md

+### Exceptions / False Positives
+
+- Allow explicit smoke tests when the purpose is only to verify the component/function does not throw on render or initialization.
+- Do not flag helper tests that intentionally validate test utilities/builders rather than product behavior, if the subject under test is the utility itself.


I am not sure I understand this rule. Are we ever testing our test utilities?

labkey-alan · 2026-03-02T17:21:20Z

.agents/review-checklists/jest/business-logic.md

+expect(data[0].name).toBe('Alpha'); // This tests your test, not your code
+
+// ✅ GOOD — asserts on rendered output from that input
+const data = [{ name: 'Alpha', value: 10 }];


I wonder if we should encourage even more specific checks here. I think keeping this section is good, but maybe adding something that says "instead of looking for the existence of data anywhere, look for the existence of data where it is expected to be". Then provide an example of looking for "name" to be in the first column of the example Table component here, or something similar. Just generically looking for content to be on the page may be so overbroad that it is not useful.

labkey-alan · 2026-03-02T18:11:25Z

.agents/review-checklists/jest/code-quality.md

+
+- Do not flag short tests (roughly 3-5 lines) where Arrange/Act/Assert is obvious without comments.
+- Do not require AAA comments in parameterized tests when added comments make the test harder to scan than the code itself.
+- Prefer suggestions over high-severity findings unless the repository explicitly enforces AAA comment structure in tests.


I am not sure this is useful, since we don't have any repositories where AAA comments are explicitly enforced, and most of our existing tests do not follow this pattern.

labkey-alan · 2026-03-02T18:40:45Z

.agents/review-checklists/react/business-logic.md

+- **Before:** `items.map((item, index) => <Row key={index} ... />)`
+- **After:** `items.map(item => <Row key={item.id} ... />)`
+
+If no natural unique key exists, consider whether the data model should include one. As a last resort, generate stable IDs when the data is first loaded.


This line feels redundant given the (IMO) more thorough instructions in the Exceptions / False Positives section above.

labkey-alan · 2026-03-02T18:42:32Z

.agents/review-checklists/react/business-logic.md

+{count > 0 && <Badge count={count} />}
+
+// ✅ Or use a ternary
+{count ? <Badge count={count} /> : null}


This pattern is always worse IMO and I don't think we should offer it as an acceptable alternative. The count > 0 && solution above is less verbose and more explicit, which is better.

labkey-alan · 2026-03-02T18:50:11Z

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

+
+- Do not flag tiny local components with a single simple prop where an inline type is shorter and clearer.
+- Do not require this pattern if the surrounding file consistently uses a different accepted convention and the PR is not refactoring style.
+- If the project avoids `FC` entirely and types props on the function parameter instead, apply the spirit of the rule (named props type) rather than the exact syntax.


I think we should remove this rule. We require components to be typed, so if someone made a component that was not labeled with FC I would consider that bad, and would request that is fixed prior to merging.

labkey-alan · 2026-03-02T18:54:03Z

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

+2. For each optional prop, verify whether any call site omits it
+3. If all call sites provide the prop, flag it for conversion to required
+
+**Scope note:** For widely-used shared components (5+ call sites), this check can be deferred to a separate audit. Focus on components with 1–3 usages where the call sites are visible in the current PR.


Suggested change

**Scope note:** For widely-used shared components (5+ call sites), this check can be deferred to a separate audit. Focus on components with 1–3 usages where the call sites are visible in the current PR.

**Scope note:** For widely-used shared components (5+ call sites), this check can be deferred to a separate audit. Focus on components with 1–4 usages where the call sites are visible in the current PR.

labkey-alan · 2026-03-02T18:57:12Z

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

+
+### Confidence Threshold
+
+Flag only when the file/area already uses a class-name utility convention (for example, `classnames`, `clsx`, or local `cn`) and the inline conditional harms consistency/readability. For one-off simple cases in mixed-style files, prefer a suggestion.


We only use classnames, and unless someone has a good reason to add a new dependency, I don't see that changing

labkey-alan · 2026-03-02T19:17:12Z

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

+
+### Confidence Threshold
+
+Flag when the computation is non-trivial (for example, sorting/filtering large collections or repeated transformations) and can run frequently with unchanged inputs. For small arrays or infrequent renders, make this a suggestion only.


Suggested change

Flag when the computation is non-trivial (for example, sorting/filtering large collections or repeated transformations) and can run frequently with unchanged inputs. For small arrays or infrequent renders, make this a suggestion only.

Flag when the computation is non-trivial (for example, sorting/filtering large collections or repeated transformations) and can run frequently with unchanged inputs. For small arrays or infrequent renders, make this a suggestion only. If array size is not known, prefer memoization.

labkey-alan · 2026-03-02T19:36:44Z

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

+
+### Confidence Threshold
+
+Flag as a high-severity finding when a nested function is used as a React component type (capitalized and rendered with JSX) inside a component body, causing remounts and state loss. Do not confuse this with render helpers that return JSX but are not treated as component types.


I think we should consider a rule that discourages render helpers. We have relied on them a ton in the past, but not in helpful ways. 99/100 render helpers at LabKey should be components.

frontend skills

93d3c09

labkey-martyp requested review from cnathe, labkey-alan and labkey-jeckels February 23, 2026 13:47

labkey-jeckels approved these changes Feb 26, 2026

View reviewed changes

cnathe approved these changes Mar 2, 2026

View reviewed changes

labkey-alan approved these changes Mar 2, 2026

View reviewed changes


		### Confidence Threshold

		Flag when the symbol is clearly unused in the file or when a mock setup is not consumed by behavior under test. If a mock or import may be used implicitly (module mocking side effects, global setup conventions), verify before commenting.


		> Prerequisite: Review and apply the common guidelines in [`common.md`](../common.md) before using this checklist.

		## No redundant or near-duplicate tests

	Scope note: For widely-used shared components (5+ call sites), this check can be deferred to a separate audit. Focus on components with 1–3 usages where the call sites are visible in the current PR.
	Scope note: For widely-used shared components (5+ call sites), this check can be deferred to a separate audit. Focus on components with 1–4 usages where the call sites are visible in the current PR.


		### Confidence Threshold

		Flag only when the file/area already uses a class-name utility convention (for example, `classnames`, `clsx`, or local `cn`) and the inline conditional harms consistency/readability. For one-off simple cases in mixed-style files, prefer a suggestion.


		### Confidence Threshold

		Flag when the computation is non-trivial (for example, sorting/filtering large collections or repeated transformations) and can run frequently with unchanged inputs. For small arrays or infrequent renders, make this a suggestion only.


		### Confidence Threshold

		Flag as a high-severity finding when a nested function is used as a React component type (capitalized and rendered with JSX) inside a component body, causing remounts and state loss. Do not confuse this with render helpers that return JSX but are not treated as component types.

Conversation

labkey-martyp commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Changes

Uh oh!

labkey-jeckels left a comment

Choose a reason for hiding this comment

Uh oh!

cnathe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labkey-alan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

labkey-martyp commented Feb 23, 2026 •

edited

Loading