Open
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
📝 WalkthroughWalkthroughThis PR adds comprehensive Jest test suites for HTML-to-JSX and resume builder components, refactors the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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 (2)
src/app/resume-builder/ResumeForm.jsx (2)
108-119: Consider removing or uncommenting the disabledaddSkillfunctionality.These commented-out blocks (here and lines 589-598) create noise. If the feature is planned, a TODO comment explaining intent would be clearer; otherwise, consider removing the dead code since it can be recovered from version control.
🤖 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 108 - 119, The commented-out addSkill block is dead code and should be either removed or restored with intent: either delete the commented addSkill (and the similar block at lines ~589-598) to eliminate noise, or uncomment and reintroduce the function (ensure you export/use addSkill where needed and it correctly calls setFormData with formData.skills to add the default categories) and add a single-line TODO comment above it explaining planned behavior; reference the addSkill identifier and setFormData/formData.skills so you update the right code paths.
10-14: Guard against unintended resets from reference changes.If the parent component re-creates
initialDataon each render (e.g., inline object or computed value without memoization), every render will trigger this effect and overwrite user edits. Consider deep-comparing or expecting the parent to memoizeinitialData.Additionally, if
initialDatabecomesundefinedafter previously being set, local state won't revert todefaultResumeData.💡 Possible approach: add a controlled vs uncontrolled mode
useEffect(() => { - if (initialData) { + // Only sync on mount or when explicitly reset by parent + // Consider using a separate `resetKey` prop if reset behavior is needed + if (initialData && !hasUserEdited) { setFormData(initialData); } }, [initialData]);Alternatively, document that
initialDatamust be memoized by the parent or implement a comparison check.🤖 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 effect that sets local state from initialData (useEffect that calls setFormData(initialData)) currently overwrites user edits whenever the parent provides a new object reference; change it to only update when initialData is meaningfully different or when initializing: in the useEffect surrounding setFormData, add a guard that (a) returns early if initialData is undefined, (b) compares initialData against current formData with a deep-equality check (e.g., lodash/isEqual or JSON.stringify) and only calls setFormData when they differ, and (c) optionally use a ref like hasInitialized to allow a one-time initialization and reset to defaultResumeData if initialData becomes explicitly undefined — update the effect that references initialData, setFormData, formData, defaultResumeData, and add the hasInitialized ref and isEqual import as needed.
🤖 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: The current cacheRef uses a plain object (const cacheRef =
useRef({})) which allows inherited-property collisions (e.g., "toString") when
reading cacheRef.current[value]; replace it with a Map by initializing cacheRef
= useRef(new Map()) and update all accesses in this file (the cache get/set
calls around the conversion logic referenced as cacheRef.current[value] on lines
~27-33) to use cacheRef.current.get(value) and cacheRef.current.set(value,
result) so keys are treated safely and no inherited properties can be returned.
---
Nitpick comments:
In `@src/app/resume-builder/ResumeForm.jsx`:
- Around line 108-119: The commented-out addSkill block is dead code and should
be either removed or restored with intent: either delete the commented addSkill
(and the similar block at lines ~589-598) to eliminate noise, or uncomment and
reintroduce the function (ensure you export/use addSkill where needed and it
correctly calls setFormData with formData.skills to add the default categories)
and add a single-line TODO comment above it explaining planned behavior;
reference the addSkill identifier and setFormData/formData.skills so you update
the right code paths.
- Around line 10-14: The effect that sets local state from initialData
(useEffect that calls setFormData(initialData)) currently overwrites user edits
whenever the parent provides a new object reference; change it to only update
when initialData is meaningfully different or when initializing: in the
useEffect surrounding setFormData, add a guard that (a) returns early if
initialData is undefined, (b) compares initialData against current formData with
a deep-equality check (e.g., lodash/isEqual or JSON.stringify) and only calls
setFormData when they differ, and (c) optionally use a ref like hasInitialized
to allow a one-time initialization and reset to defaultResumeData if initialData
becomes explicitly undefined — update the effect that references initialData,
setFormData, formData, defaultResumeData, and add the hasInitialized ref and
isEqual import as needed.
🪄 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: 428dcaa4-33b8-457c-88a2-a4485de44f0c
📒 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.
Use Map for cache keys to avoid inherited-property collisions.
At Line 27, cacheRef.current[value] can read inherited object properties (e.g., "toString"), returning non-JSX data and breaking conversion. This is user-input-driven and should be hardened.
🔧 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;
};Also applies to: 27-33
🤖 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, The current cacheRef uses a
plain object (const cacheRef = useRef({})) which allows inherited-property
collisions (e.g., "toString") when reading cacheRef.current[value]; replace it
with a Map by initializing cacheRef = useRef(new Map()) and update all accesses
in this file (the cache get/set calls around the conversion logic referenced as
cacheRef.current[value] on lines ~27-33) to use cacheRef.current.get(value) and
cacheRef.current.set(value, result) so keys are treated safely and no inherited
properties can be returned.
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
1 participant
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 for the HTML-to-JSX conversion module.
This PR focuses only on the HTML-to-JSX module for better review.
Summary by CodeRabbit
New Features
Tests
Documentation
Refactor