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.

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

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.

Yeah good call. I think some of this can be combined. I will look into doing that in a follow up PR.

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?

Yeah it kind of applies to variables, imports or mock setups. I'm not sure if there's more appropriate terminology?

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!

Yeah I think these kinds of things can save us time on code reviews so we can spend more time on higher level thinking.

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.

Yeah maybe "redundant" and "near-duplicate" should be split into separate checks. One is performance - not running redundant tests, the other may be more code-quality - sharing common code and using test.each (see example)

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.

Yeah these can be annoying to debug

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

Yes it absolutely can. So much easier than doing it 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?

I removed it. I think it would be applicable if we were specifically testing our API wrapper framework or something, but probably not doing that anywhere.

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.

Ok I added a more specific example.

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.

I found it really useful to at least have the Assert part with a comment on how it is asserting what is being tested. Some structure and consistency is helpful as well. But I'm not set on AAA, it's just what I found when researching ways to have test clarity for people and AI.

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

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

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

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.

Can you point me to an example or two in our code? We can get that in a follow up PR.

Co-authored-by: Alan Vezina <alanv@labkey.com>

…abKey/server into fb_frontend_code_review_skills

labkey-martyp · 2026-03-04T09:45:19Z

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.

Yep it would be great to discuss a better location. I actually thought everyone was putting labkey-ui-components and labkey-ui-premium under server, but from our meeting I now realize that is not the case.

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

labkey-martyp and others added 5 commits March 4, 2026 01:28

CR feedback

7de5fee

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

6e43680

Co-authored-by: Alan Vezina <alanv@labkey.com>

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

497b9b3

Co-authored-by: Alan Vezina <alanv@labkey.com>

few more CR updates

4a8d57a

Merge branch 'fb_frontend_code_review_skills' of https://github.com/L…

39761b2

…abKey/server into fb_frontend_code_review_skills

Merge branch 'develop' into fb_frontend_code_review_skills

09c70fb

labkey-martyp merged commit 7605253 into develop Mar 4, 2026
6 of 7 checks passed

labkey-martyp deleted the fb_frontend_code_review_skills branch March 4, 2026 10:37


		### 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


		### 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!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labkey-martyp commented Mar 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

labkey-martyp commented Feb 23, 2026 •

edited

Loading