Add Jest tests for HTML-to-JSX tool and update README instructions#625
Closed
ankit20012006 wants to merge 2 commits intoBashamega:mainfrom
Closed
Add Jest tests for HTML-to-JSX tool and update README instructions#625ankit20012006 wants to merge 2 commits intoBashamega:mainfrom
ankit20012006 wants to merge 2 commits intoBashamega:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request adds comprehensive test coverage for the HTML-to-JSX converter and Resume Builder components, extracts default resume data into a dedicated module, and refactors state management in the Resume Form and converter components for improved maintainability and efficiency. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@ankit20012006 is attempting to deploy a commit to the bashamega's projects Team on Vercel. A member of the Team first needs to authorize it. |
Bashamega
requested changes
Apr 9, 2026
Owner
Bashamega
left a comment
Bashamega
left a comment
There was a problem hiding this comment.
Please split this to smaller PRS. Each module in a separate PR
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
__tests__/resume-builder/ResumeForm.test.jsx (1)
71-79: Test assertion may be fragile if multiple updates occur.Line 78 assumes
onFormChangewas called exactly once (mock.calls[0]). If the component implementation changes to trigger multiple calls (e.g., during mount or re-render), this assertion could break or give a false positive. Consider usingtoHaveBeenLastCalledWithor checking the final call explicitly.♻️ Suggested improvement
- expect(onFormChange.mock.calls[0][0].workExperience).toHaveLength(2); + const lastCall = onFormChange.mock.calls[onFormChange.mock.calls.length - 1][0]; + expect(lastCall.workExperience).toHaveLength(2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/resume-builder/ResumeForm.test.jsx` around lines 71 - 79, The assertion that inspects onFormChange.mock.calls[0][0].workExperience is fragile because it assumes a single call; update the test to assert against the final call instead (use onFormChange.toHaveBeenLastCalledWith(...) or read the last call via onFormChange.mock.calls[onFormChange.mock.calls.length - 1][0]) and then check that .workExperience has length 2, keeping the existing expect.objectContaining shape check for workExperience; reference the onFormChange mock used in ResumeForm.test.jsx to locate and update the assertion.src/app/convert/html-jsx/page.jsx (2)
45-48: MissinggenerateJSXin useMemo dependency array.The
useMemohook callsgenerateJSXbut doesn't list it as a dependency. While this works correctly (sincegenerateJSXonly closes over the stablecacheRefandvaluewhich is in deps), ESLint'sexhaustive-depsrule may flag this. Consider inlining the logic or wrappinggenerateJSXinuseCallback.♻️ Option: Inline the cache logic
- // Generate JSX from HTML with caching - const generateJSX = () => { - if (cacheRef.current[value]) { - return cacheRef.current[value]; - } - - const jsx = htmlToJsx(value); - const jsxCode = `function component() { return (${jsx}) }`; - cacheRef.current[value] = jsxCode; - return jsxCode; - }; - - // Effect to load theme from localStorage on mount - useEffect(() => { - const storedTheme = JSON.parse(localStorage.getItem("theme")); - if (storedTheme !== null) { - setIsDarkMode(storedTheme); - } - }, []); - - // Memoize the generated JSX to avoid recomputation on every render - const generatedJSX = useMemo(() => { - return generateJSX(); - }, [value]); + // Effect to load theme from localStorage on mount + useEffect(() => { + const storedTheme = JSON.parse(localStorage.getItem("theme")); + if (storedTheme !== null) { + setIsDarkMode(storedTheme); + } + }, []); + + // Memoize the generated JSX with caching + const generatedJSX = useMemo(() => { + if (cacheRef.current[value]) { + return cacheRef.current[value]; + } + const jsx = htmlToJsx(value); + const jsxCode = `function component() { return (${jsx}) }`; + cacheRef.current[value] = jsxCode; + return jsxCode; + }, [value]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/convert/html-jsx/page.jsx` around lines 45 - 48, The useMemo currently depends only on value but calls generateJSX, which ESLint's exhaustive-deps will flag; fix by either inlining the generateJSX logic into the useMemo callback so there is no external function dependency, or wrap generateJSX with useCallback (depending on cacheRef and value) and then include generateJSX in the useMemo dependency array; specifically update the component so generateJSX is memoized via useCallback (or removed by inlining its logic) and ensure the useMemo deps include generateJSX (or only value if inlined).
26-35: Unbounded cache growth.The cache (
cacheRef.current) grows indefinitely as users input different HTML strings. For a typical session this is fine, but consider whether a bounded cache (LRU or size-limited) would be more appropriate for extended use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/convert/html-jsx/page.jsx` around lines 26 - 35, The generateJSX function uses cacheRef.current[value] which allows unbounded growth; replace this with a bounded cache (e.g., an LRU or size-limited Map) to cap entries and evict old ones when a MAX_CACHE_SIZE is exceeded. Implement a small LRU helper (or use a Map with insertion order handling) and update generateJSX to check the bounded cache for value, move accessed entries to most-recent position, insert new entries, and evict the oldest key when size > MAX_CACHE_SIZE; reference the symbols generateJSX, cacheRef.current, value, and htmlToJsx when making the change.README.md (1)
62-65: Minor formatting inconsistency.Step 5 is missing a blank line before the code block, unlike the other steps. This doesn't affect functionality but breaks visual consistency.
📝 Suggested fix
5. **Run Coverage Report**: + ```bash npm run test:coverage ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 62 - 65, The markdown for step "5. **Run Coverage Report**" has no blank line before its fenced code block; open the README and add a single blank line immediately before the triple-backtick block that contains "npm run test:coverage" so the formatting matches the other steps and preserves consistent spacing around code blocks.__tests__/convert/html-jsx/page.test.jsx (1)
52-55: Make conversion assertion less brittle to harmless formatting changes.At Line 53, asserting the full generated function string tightly couples the test to exact output formatting.
♻️ Suggested assertion update
- await waitFor(() => { - expect(jsxEditor).toHaveValue( - 'function component() { return (<div className="foo">Hello</div>) }', - ); - }); + await waitFor(() => { + const jsxValue = screen.getByLabelText("JSX editor").value; + expect(jsxValue).toContain('className="foo"'); + expect(jsxValue).toContain("Hello"); + expect(jsxValue).toContain("<div"); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/convert/html-jsx/page.test.jsx` around lines 52 - 55, The test is brittle because it asserts the exact serialized function string in the jsxEditor value; instead modify the assertion in the waitFor block (where jsxEditor and toHaveValue are used) to verify the important content rather than exact formatting — e.g., check that jsxEditor contains the component name and the JSX snippet (like "function component" and '<div className="foo">Hello</div>') or use a loose regex/whitespace-normalization before asserting, so harmless formatting changes won't break the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/resume-builder/ResumeForm.jsx`:
- Around line 10-14: The useEffect that resets formData from initialData causes
a bidirectional sync loop between the child and parent; change the component so
initialData is only used to initialize local state (remove the useEffect that
calls setFormData(initialData)) or convert the component to fully controlled by
removing internal state and calling onFormChange directly from inputs; update
references where local state is set/used (setFormData, formData, updateFormData)
and ensure onFormChange is still invoked with nextData when inputs change if you
keep local state.
---
Nitpick comments:
In `@__tests__/convert/html-jsx/page.test.jsx`:
- Around line 52-55: The test is brittle because it asserts the exact serialized
function string in the jsxEditor value; instead modify the assertion in the
waitFor block (where jsxEditor and toHaveValue are used) to verify the important
content rather than exact formatting — e.g., check that jsxEditor contains the
component name and the JSX snippet (like "function component" and '<div
className="foo">Hello</div>') or use a loose regex/whitespace-normalization
before asserting, so harmless formatting changes won't break the test.
In `@__tests__/resume-builder/ResumeForm.test.jsx`:
- Around line 71-79: The assertion that inspects
onFormChange.mock.calls[0][0].workExperience is fragile because it assumes a
single call; update the test to assert against the final call instead (use
onFormChange.toHaveBeenLastCalledWith(...) or read the last call via
onFormChange.mock.calls[onFormChange.mock.calls.length - 1][0]) and then check
that .workExperience has length 2, keeping the existing expect.objectContaining
shape check for workExperience; reference the onFormChange mock used in
ResumeForm.test.jsx to locate and update the assertion.
In `@README.md`:
- Around line 62-65: The markdown for step "5. **Run Coverage Report**" has no
blank line before its fenced code block; open the README and add a single blank
line immediately before the triple-backtick block that contains "npm run
test:coverage" so the formatting matches the other steps and preserves
consistent spacing around code blocks.
In `@src/app/convert/html-jsx/page.jsx`:
- Around line 45-48: The useMemo currently depends only on value but calls
generateJSX, which ESLint's exhaustive-deps will flag; fix by either inlining
the generateJSX logic into the useMemo callback so there is no external function
dependency, or wrap generateJSX with useCallback (depending on cacheRef and
value) and then include generateJSX in the useMemo dependency array;
specifically update the component so generateJSX is memoized via useCallback (or
removed by inlining its logic) and ensure the useMemo deps include generateJSX
(or only value if inlined).
- Around line 26-35: The generateJSX function uses cacheRef.current[value] which
allows unbounded growth; replace this with a bounded cache (e.g., an LRU or
size-limited Map) to cap entries and evict old ones when a MAX_CACHE_SIZE is
exceeded. Implement a small LRU helper (or use a Map with insertion order
handling) and update generateJSX to check the bounded cache for value, move
accessed entries to most-recent position, insert new entries, and evict the
oldest key when size > MAX_CACHE_SIZE; reference the symbols generateJSX,
cacheRef.current, value, and htmlToJsx when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07731f92-3ffb-4f5d-9c4a-cea8e9b15000
📒 Files selected for processing (9)
README.md__tests__/convert/html-jsx/htmlToJsx.test.js__tests__/convert/html-jsx/page.test.jsx__tests__/resume-builder/Preview.test.jsx__tests__/resume-builder/ResumeForm.test.jsxsrc/app/convert/html-jsx/page.jsxsrc/app/resume-builder/ResumeForm.jsxsrc/app/resume-builder/defaultResumeData.jssrc/app/resume-builder/page.jsx
Comment on lines
+10
to
+14
| useEffect(() => { | ||
| if (initialData) { | ||
| setFormData(initialData); | ||
| } | ||
| }, [initialData]); |
Contributor
There was a problem hiding this comment.
Potential issue: bidirectional sync between parent and child state.
The useEffect sets formData whenever initialData changes. Since onFormChange updates the parent's resumeData (which is passed back as initialData), this creates a feedback loop:
- User types →
updateFormData→onFormChange(nextData)→ parent setsresumeData - New
resumeDatapassed asinitialData→useEffectfires →setFormData(initialData)
While React may optimize away redundant state updates, this pattern risks overwriting in-progress user input in edge cases (e.g., rapid typing with async parent updates). Consider either:
- Making
ResumeFormfully controlled (no internal state, all state in parent) - Or making it fully uncontrolled (only use
initialDatafor initial mount)
♻️ Option: Use initialData only on mount
useEffect(() => {
- if (initialData) {
- setFormData(initialData);
- }
- }, [initialData]);
+ // Only sync on mount - remove this effect entirely if not needed
+ }, []);Or remove the useEffect entirely since useState(initialData ?? defaultResumeData) already handles initialization.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| if (initialData) { | |
| setFormData(initialData); | |
| } | |
| }, [initialData]); | |
| useEffect(() => { | |
| // Only sync on mount - remove this effect entirely if not needed | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/resume-builder/ResumeForm.jsx` around lines 10 - 14, The useEffect
that resets formData from initialData causes a bidirectional sync loop between
the child and parent; change the component so initialData is only used to
initialize local state (remove the useEffect that calls
setFormData(initialData)) or convert the component to fully controlled by
removing internal state and calling onFormChange directly from inputs; update
references where local state is set/used (setFormData, formData, updateFormData)
and ensure onFormChange is still invoked with nextData when inputs change if you
keep local state.
Owner
|
Hey @ankit20012006 |
Author
|
@Bashamega okay i will do |
Author
|
Thanks for the feedback! I’ve split the changes into smaller PRs:
Kindly review them. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR improves test coverage and refactors state management in the HTML-to-JSX tool and Resume Builder components.
Changes Made
1. Jest Tests
2. HTML-to-JSX Tool Refactor
useStatecaching withuseReffor better performance and avoiding unnecessary re-renders.3. Resume Builder Refactor
defaultResumeData.jsmodule.ResumeFormto use a reusableupdateFormDatahelper with functional updates.initialDataprop toResumeFormfor better parent-child data flow.page.jsxto use the new default data module and pass it as a prop.4. README Update
How to Test
npm installnpm testnpm run test:coverageType
Checklist
Summary by CodeRabbit
Documentation
Tests
Improvements