Add React guidance for unit tests, JSX structure, and memoization#1353
Add React guidance for unit tests, JSX structure, and memoization#1353labkey-jeckels wants to merge 9 commits intodevelopfrom
Conversation
labkey-martyp
left a comment
labkey-martyp
left a comment
There was a problem hiding this comment.
I haven't tried it out, but looks like a good rule.
|
|
||
| ### Suggested Fix | ||
|
|
||
| Create `Component.test.tsx` with meaningful assertions per [`jest/business-logic.md`](../../jest/business-logic.md): |
There was a problem hiding this comment.
Shouldn't this path just be ../jest/business-logic.md?
| // ❌ Custom hook with async logic but no tests | ||
| const useCustomFetch = (url: string) => { | ||
| const [data, setData] = useState(null); | ||
| useEffect(() => { | ||
| fetch(url).then(res => res.json()).then(setData); | ||
| }, [url]); | ||
| return data; | ||
| }; |
There was a problem hiding this comment.
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;
};
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Updated to async function and a comment to say things were omitted for brevity. Does that seem reasonable to you both?
…eactPlateDesigner
|
I pushed some more updates based on feedback from the plate designer migration. My understanding is that 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-jeckels
changed the title
labkey-martyp
left a comment
labkey-martyp
left a comment
There was a problem hiding this comment.
Just the one merge issue, otherwise looks good.
Fixed. I'll wait for @labkey-alan to have a chance to review too. |
4 participants
Rationale
We like unit tests and well structured JSX/TSX. Make sure Claude Code does too.
Related Pull Requests
Changes