test: add resume builder tests and state fixes#627
Open
ankit20012006 wants to merge 2 commits intoBashamega:mainfrom
Open
test: add resume builder tests and state fixes#627ankit20012006 wants to merge 2 commits intoBashamega:mainfrom
ankit20012006 wants to merge 2 commits intoBashamega:mainfrom
Conversation
|
@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. |
Contributor
📝 WalkthroughWalkthroughThe PR introduces comprehensive Jest test coverage for resume builder and HTML-to-JSX components, updates documentation with test instructions, and refactors resume builder components to accept initial data props while extracting default data into a reusable module. Changes
Possibly related issues
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 |
![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)
src/app/resume-builder/ResumeForm.jsx (2)
10-14: Potential unnecessary re-renders from useEffect sync.When user types,
updateFormDatacallsonFormChangewith the new data. Parent merges and updatesresumeData, which changesinitialDataprop. This triggers the useEffect, callingsetFormData(initialData)again with essentially the same data, causing an extra render cycle.Consider memoizing
initialDatain the parent or adding a shallow equality check before callingsetFormData:💡 Add equality check to avoid redundant updates
useEffect(() => { - if (initialData) { + if (initialData && initialData !== formData) { setFormData(initialData); } - }, [initialData]); + }, [initialData, formData]);Or, if
initialDatais only meant to seed the form once:- useEffect(() => { - if (initialData) { - setFormData(initialData); - } - }, [initialData]); + // Remove useEffect entirely - initial state handles seeding🤖 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 syncs props to state (useEffect -> setFormData(initialData)) causes redundant renders because parent updates (via updateFormData -> onFormChange -> resumeData) re-pass essentially identical initialData; update the effect to avoid calling setFormData when data is shallowly equal to current formData (compare keys/values) or only seed once (use a seeded ref flag) so setFormData(initialData) runs only when initialData actually differs; reference the useEffect, setFormData, initialData, updateFormData, onFormChange and resumeData to locate and apply the shallow equality check or the "seed once" guard.
16-25: CallingonFormChangeinside the setState updater is unconventional.While functional, invoking callbacks inside the state updater can lead to subtle timing issues if the parent's state update affects this component before the current render commits. A safer pattern is to call
onFormChangeaftersetFormData:💡 Alternative pattern
const updateFormData = (updater) => { - setFormData((prevData) => { - const nextData = - typeof updater === "function" - ? updater(prevData) - : { ...prevData, ...updater }; - onFormChange?.(nextData); - return nextData; - }); + setFormData((prevData) => { + const nextData = + typeof updater === "function" + ? updater(prevData) + : { ...prevData, ...updater }; + return nextData; + }); }; + + useEffect(() => { + onFormChange?.(formData); + }, [formData, onFormChange]);This separates state updates from side effects, following React's recommended patterns.
🤖 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 16 - 25, The updateFormData function currently calls onFormChange inside the setFormData updater; change this to avoid side effects in the updater by removing onFormChange from updateFormData (leave setFormData usage as-is) and instead add a useEffect that watches formData and calls onFormChange?.(formData) whenever formData changes; reference updateFormData and setFormData in the change and add the new useEffect(() => { onFormChange?.(formData) }, [formData]) so state updates and side effects are separated.__tests__/resume-builder/ResumeForm.test.jsx (1)
78-78: Consider using a more robust assertion for the call index.
mock.calls[0]assumes this is the first call, which can be brittle if the component's behavior changes. Using the last call or finding the specific call would be more resilient.💡 More resilient assertion
- expect(onFormChange.mock.calls[0][0].workExperience).toHaveLength(2); + const lastCall = onFormChange.mock.calls[onFormChange.mock.calls.length - 1]; + expect(lastCall[0].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` at line 78, The test currently asserts using onFormChange.mock.calls[0][0].workExperience which assumes the change happened on the first call; update the assertion to reference the last (or specific) call to make it resilient — e.g., use onFormChange.mock.lastCall[0].workExperience or compute const lastCall = onFormChange.mock.calls[onFormChange.mock.calls.length - 1] and assert lastCall[0].workExperience toHaveLength(2); keep the assertion targeting the onFormChange mock used in this test.src/app/resume-builder/defaultResumeData.js (1)
1-20: Consider making the default data immutable or returning a factory function.The exported object is mutable and shared across all imports. While current handlers use spread operators (safe), direct mutations elsewhere could cause subtle bugs. A factory function guarantees fresh copies:
💡 Factory function approach
-export const defaultResumeData = { +export const createDefaultResumeData = () => ({ name: "", email: "", phone: "", address: "", image: "", imageShape: "circle", workExperience: [{ title: "", company: "", description: "" }], projects: [{ title: "", liveUrl: "", description: "" }], education: [{ degree: "", institution: "", description: "" }], skills: [ { category: "Frontend", skills: [""] }, { category: "Backend", skills: [""] }, { category: "Languages", skills: [""] }, { category: "Tools", skills: [""] }, ], links: { linkedIn: "", website: "", github: "" }, achievements: [{ title: "", description: "" }], template: "template1", -}; +}); + +// For backward compatibility +export const defaultResumeData = createDefaultResumeData();Then use
createDefaultResumeData()inuseStatecalls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/resume-builder/defaultResumeData.js` around lines 1 - 20, The exported defaultResumeData is a single mutable object shared across imports; replace it with a factory function (e.g., export function createDefaultResumeData()) that returns a new deep-fresh object for each call (ensure nested arrays/objects like workExperience, projects, education, skills, links, achievements are newly constructed), and update callers (e.g., useState initializers) to call createDefaultResumeData() instead of referencing defaultResumeData; optionally keep an exported frozen constant only for reference but do not use it as the state source.__tests__/resume-builder/Preview.test.jsx (1)
27-32: Consider adding assertion for the GitHub link.The
sampleData.links.githubis defined but never verified in assertions, unlike LinkedIn and website links.💡 Optional addition
expect( screen.getByText((content) => content.includes("https://janedoe.dev")), ).toBeInTheDocument(); + expect( + screen.getByText((content) => content.includes("github.com/janedoe")), + ).toBeInTheDocument(); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/resume-builder/Preview.test.jsx` around lines 27 - 32, The test defines sampleData.links.github but never asserts it; update the test in Preview.test.jsx (where sampleData and link assertions live) to include a GitHub link assertion—locate the existing assertions for linkedIn and website and add a similar expect that queries the GitHub anchor (e.g., by role/name or text) and asserts its href equals sampleData.links.github so the GitHub URL is validated alongside the other links.
🤖 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/convert/html-jsx/page.jsx`:
- Line 11: cacheRef is initialized as useRef({}) which allows inherited-object
keys like "constructor" to collide; change its initialization to useRef(new
Map()) and replace all direct property access/assignment
(cacheRef.current[value], cacheRef.current[value] = something) with Map methods:
use cacheRef.current.has(value) to check, cacheRef.current.get(value) to read,
and cacheRef.current.set(value, something) to store; update any conditional
logic that relied on truthy property checks to use .has(...) instead.
---
Nitpick comments:
In `@__tests__/resume-builder/Preview.test.jsx`:
- Around line 27-32: The test defines sampleData.links.github but never asserts
it; update the test in Preview.test.jsx (where sampleData and link assertions
live) to include a GitHub link assertion—locate the existing assertions for
linkedIn and website and add a similar expect that queries the GitHub anchor
(e.g., by role/name or text) and asserts its href equals sampleData.links.github
so the GitHub URL is validated alongside the other links.
In `@__tests__/resume-builder/ResumeForm.test.jsx`:
- Line 78: The test currently asserts using
onFormChange.mock.calls[0][0].workExperience which assumes the change happened
on the first call; update the assertion to reference the last (or specific) call
to make it resilient — e.g., use onFormChange.mock.lastCall[0].workExperience or
compute const lastCall = onFormChange.mock.calls[onFormChange.mock.calls.length
- 1] and assert lastCall[0].workExperience toHaveLength(2); keep the assertion
targeting the onFormChange mock used in this test.
In `@src/app/resume-builder/defaultResumeData.js`:
- Around line 1-20: The exported defaultResumeData is a single mutable object
shared across imports; replace it with a factory function (e.g., export function
createDefaultResumeData()) that returns a new deep-fresh object for each call
(ensure nested arrays/objects like workExperience, projects, education, skills,
links, achievements are newly constructed), and update callers (e.g., useState
initializers) to call createDefaultResumeData() instead of referencing
defaultResumeData; optionally keep an exported frozen constant only for
reference but do not use it as the state source.
In `@src/app/resume-builder/ResumeForm.jsx`:
- Around line 10-14: The useEffect that syncs props to state (useEffect ->
setFormData(initialData)) causes redundant renders because parent updates (via
updateFormData -> onFormChange -> resumeData) re-pass essentially identical
initialData; update the effect to avoid calling setFormData when data is
shallowly equal to current formData (compare keys/values) or only seed once (use
a seeded ref flag) so setFormData(initialData) runs only when initialData
actually differs; reference the useEffect, setFormData, initialData,
updateFormData, onFormChange and resumeData to locate and apply the shallow
equality check or the "seed once" guard.
- Around line 16-25: The updateFormData function currently calls onFormChange
inside the setFormData updater; change this to avoid side effects in the updater
by removing onFormChange from updateFormData (leave setFormData usage as-is) and
instead add a useEffect that watches formData and calls onFormChange?.(formData)
whenever formData changes; reference updateFormData and setFormData in the
change and add the new useEffect(() => { onFormChange?.(formData) }, [formData])
so state updates and side effects are separated.
🪄 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: af3e226c-8c95-4703-8f42-25af2d5ec251
📒 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
| const [isDarkMode, setIsDarkMode] = useState(false); | ||
| const [value, setValue] = useState(""); | ||
| const [cache, setCache] = useState({}); | ||
| const cacheRef = useRef({}); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the file exists and read it
cat -n src/app/convert/html-jsx/page.jsxRepository: Bashamega/WebDevTools
Length of output: 2893
Use a Map for cache keys to avoid inherited-property collisions.
On line 27, cacheRef.current[value] can incorrectly hit inherited keys (e.g., constructor, toString) and return non-JSX values, breaking output for certain inputs. Use Map with .has() and .get() for safe key lookups.
Proposed fix
- const cacheRef = useRef({});
+ const cacheRef = useRef(new Map());
// Generate JSX from HTML with caching
const generateJSX = () => {
- if (cacheRef.current[value]) {
- return cacheRef.current[value];
+ if (cacheRef.current.has(value)) {
+ return cacheRef.current.get(value);
}
const jsx = htmlToJsx(value);
const jsxCode = `function component() { return (${jsx}) }`;
- cacheRef.current[value] = jsxCode;
+ cacheRef.current.set(value, jsxCode);
return jsxCode;
};🤖 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` at line 11, cacheRef is initialized as
useRef({}) which allows inherited-object keys like "constructor" to collide;
change its initialization to useRef(new Map()) and replace all direct property
access/assignment (cacheRef.current[value], cacheRef.current[value] = something)
with Map methods: use cacheRef.current.has(value) to check,
cacheRef.current.get(value) to read, and cacheRef.current.set(value, something)
to store; update any conditional logic that relied on truthy property checks to
use .has(...) instead.
Owner
|
Hey @ankit20012006 |
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 adds test cases and improvements for the Resume Builder module.
This PR focuses only on the Resume Builder module for better review.
Summary by CodeRabbit
Documentation
Tests
Refactor