Add React guidance for unit tests, JSX structure, and memoization

[labkey-jeckels]

Rationale

We like unit tests and well structured JSX/TSX. Make sure Claude Code does too.

Related Pull Requests

• Reimplement GWT assay plate designer in React platform#7623

Changes

• Specific guidance for what we want to see in JSX/TSX
• Specific guidance for unit tests for React code

[labkey-martyp]

I haven't tried it out, but looks like a good rule.

[labkey-martyp]

Shouldn't this path just be ../jest/business-logic.md?

[labkey-jeckels]

Yes. Fixed.

[labkey-martyp]

Claude is flagging this as bad code example because it's missing error handling and cleanup, which could encourage an LLM to use this. Here's the recommended code.

// ❌ Custom hook with async logic but no tests
  const useCustomFetch = <T,>(url: string): T | null => {
      const [data, setData] = useState<T | null>(null);
      useEffect(() => {
          const controller = new AbortController();
          fetch(url, { signal: controller.signal })
              .then(res => res.json())
              .then(setData)
              .catch(() => { /* ignore aborts/errors for brevity */ });
          return () => controller.abort();
      }, [url]);
      return data;
  };

[labkey-jeckels]

This is interesting. It IS a bad code example, though its intent is to be bad in a different way (lacking tests, not bad error handling).

Do we need our examples of bad code to be good in ways other than the specific problem they're intending to call out? The downside is overly verbose examples and token bloat.

[labkey-alan]

I am not sure how useful these code examples are because we cannot use fetch anywhere, and as a result cannot use AbortControllers. We'd need to release a 2.x version of @labkey/api for this to be relevant to us. This is something we should do, and I was just talking with Nick about it the other day because I want to use AbortControllers, but there isn't really a path forward for us to use either because all of our API requests are done via Ajax.request

[labkey-martyp]

This was a low priority call out by claude, but I suspect what you might get is an LLM adds tests to the bad example and thinks it is now good code to use as a reference. Even though it has other issues. Maybe something to research further.

[labkey-jeckels]

Updated to async function and a comment to say things were omitted for brevity. Does that seem reasonable to you both?

[labkey-jeckels]

I pushed some more updates based on feedback from the plate designer migration.

My understanding is that CLAUDE.md is the right place to put guidance to help Claude generate new code according to our conventions. I added notes based on things it did repeatedly that I refactored prior to merging.

The guidance in the other files I think is more general purpose for all reviews, regardless of who wrote the code.

I'll re-request review based on these updates.

[labkey-martyp]

Just the one merge issue, otherwise looks good.

[labkey-jeckels]

Just the one merge issue, otherwise looks good.

Fixed. I'll wait for @labkey-alan to have a chance to review too.

[labkey-alan]

I think overall this looks good, but I did leave some feedback that I think should be included. I'm approving so you can easily make the changes and merge, but if you disagree with my feedback let's have a discussion before merging.

[labkey-alan]

I disagree with this line and think we should remove it. It's incredibly rare that a single large block of JSX being split into smaller chunks would result in excessive prop-drilling. For that you'd need a wildly complicated block of JSX that needs to be split into multiple nested components, but even in that case prop drilling is preferred to a huge unreadable, hard to test, block of JSX. Also, in those cases you can leverage things like react context to avoid prop drilling.

I also think this wording is too vague to be enforceable, what exactly qualifies as excessive? Without a clear definition of that I don't think we'd get consistent results, and I think it could easily be justified as a reason to ignore otherwise sound feedback.

If we find that not having this exception leads to prop-drilling issues in practice, we can revisit with a concrete example. For now I think just removing it is the right call.

[labkey-jeckels]

Removed.

[labkey-jeckels]

I think overall this looks good, but I did leave some feedback that I think should be included. I'm approving so you can easily make the changes and merge, but if you disagree with my feedback let's have a discussion before merging.

Thanks. I accepted your suggestions.