Issusid #230765 fix: Sonar Lint Issue in My learning Journey #216
Conversation
WalkthroughThe pull request introduces several modifications across various components in the codebase, focusing on improving code quality, readability, and maintainability. Key changes include simplifying import statements, adding PropTypes for type checking, optimizing image attributes, and refining variable declarations. Additionally, several components underwent formatting adjustments to enhance consistency and clarity. Notably, the Changes
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
src/components/Assesment/Assesment.jsxOops! Something went wrong! :( ESLint: 7.32.0 ESLint couldn't find the config "react-app" to extend from. Please check that the name of the config is correct. The config "react-app" was referenced from the config file in "/package.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (22)
src/views/HomePage/HomePage.jsx (2)
Line range hint
24-31: Remove commented out navigation code.Keeping commented out code creates maintenance overhead and can confuse future developers. If this navigation logic is no longer needed, it should be removed. If it might be needed later, consider tracking it in version control history instead.
- // let level = searchParams.get("level"); - // const navigate = useNavigate(); - // useEffect(() => { - // if (level) { - // setTimeout(() => { - // navigate("/assesment"); - // }, 2000); - // } - // }, [level]);
Line range hint
8-21: Consider extracting styles and adding parameter validation.A few suggestions to improve maintainability and robustness:
- Move inline styles to a separate CSS file
- Add validation for the level parameter
- Consider using CSS modules or styled-components for better style management
Example refactor:
+ const isValidLevel = (level) => level >= 1 && level <= 3; + const sectionStyle = { - width: "100vw", - height: "100vh", - backgroundImage: `url(${ - images?.[`desktopLevel${searchParams.get("level") || 1}`] - })`, - backgroundSize: "contain", - backgroundRepeat: "round", - position: "relative", + backgroundImage: `url(${ + images?.[`desktopLevel${isValidLevel(searchParams.get("level")) ? searchParams.get("level") : 1}`] + })` };Consider moving styles to a CSS file:
.home-section { width: 100vw; height: 100vh; background-size: contain; background-repeat: round; position: relative; }src/components/Practice/Mechanics2.jsx (2)
20-27: Consider making the layout more responsiveThe fixed padding (
padding: "20px 100px") might cause layout issues on smaller screens. Consider using responsive units or media queries.const sectionStyle = { width: "100%", backgroundImage: `url(${practicebg})`, backgroundSize: "cover", backgroundPosition: "center center", backgroundRepeat: "no-repeat", height: "100vh", - padding: "20px 100px", + padding: "20px clamp(20px, 5vw, 100px)", boxSizing: "border-box", };
74-85: Implement speech functionality for the Speak buttonThe Speak button appears to be non-functional. Consider implementing text-to-speech functionality using the Web Speech API.
Would you like me to provide an example implementation of the speech functionality or create a GitHub issue to track this task?
src/components/DiscoverEnd/DiscoverEnd.jsx (3)
52-55: Consider enhancing error handling and path management.The current implementation could benefit from:
- Better error handling with user feedback
- Centralized route management instead of hardcoded paths
Consider applying this improvement:
+ import { ROUTES } from "../../utils/constants"; // Add this to constants.js const handleProfileBack = () => { try { if (process.env.REACT_APP_IS_APP_IFRAME === "true") { - navigate("/"); + navigate(ROUTES.HOME); } else { - navigate("/discover-start"); + navigate(ROUTES.DISCOVER_START); } } catch (error) { console.error("Error posting message:", error); + // Add user feedback + toast.error("Navigation failed. Please try again."); } };
Line range hint
31-47: Consider extracting async logic from useEffect.The useEffect hook contains complex async logic that handles both audio playback and API calls. This could be split into separate effects or utility functions for better maintainability.
Consider this refactor:
+ const fetchMilestoneDetails = async (virtualId, lang) => { + const { data } = await axios.get( + `${process.env.REACT_APP_LEARNER_AI_APP_HOST}/${config.URLS.GET_MILESTONE}/${virtualId}?language=${lang}` + ); + return data.data; + }; + const playLevelCompleteAudio = () => { + const audio = new Audio(LevelCompleteAudio); + return audio.play(); + }; useEffect(() => { (async () => { - let audio = new Audio(LevelCompleteAudio); - audio.play(); + await playLevelCompleteAudio(); const virtualId = getLocalData("virtualId"); const lang = getLocalData("lang"); - const getMilestoneDetails = await axios.get( - `${process.env.REACT_APP_LEARNER_AI_APP_HOST}/${config.URLS.GET_MILESTONE}/${virtualId}?language=${lang}` - ); - const { data } = getMilestoneDetails; - setLevel(data.data.milestone_level); - setLocalData("userLevel", data.data.milestone_level?.replace("m", "")); + const milestoneData = await fetchMilestoneDetails(virtualId, lang); + setLevel(milestoneData.milestone_level); + setLocalData("userLevel", milestoneData.milestone_level?.replace("m", "")); })(); setTimeout(() => { setShake(false); }, 4000); }, []);
Line range hint
28-28: Component name doesn't match its functionality.The component is named
SpeakSentenceComponentbut appears to be a completion/end screen component. Consider renaming it to better reflect its purpose.Consider renaming to:
- const SpeakSentenceComponent = () => { + const DiscoverEndComponent = () => { - export default SpeakSentenceComponent; + export default DiscoverEndComponent;Also applies to: 153-153
src/components/AssesmentEnd/AssesmentEnd.jsx (1)
Line range hint
1-300: Consider refactoring for better maintainability.The component could benefit from several improvements:
- Consistent text styling: Currently mixing inline styles with MUI components
- Extracted constants: Move magic strings and numbers to constants
- Separated concerns: Extract the mood rating section into a separate component
- Reduced duplication: The same text styling objects are repeated
Would you like me to provide a detailed refactoring plan?
src/components/Practice/Mechanics5.jsx (3)
Line range hint
67-68: Enhance state management robustnessThe state management for selections could be improved in several ways:
- Validate selectedOption against options length
- Clean up state on unmount
- Add error handling for invalid indices
const [selectedOption, setSelectedOption] = useState(null); const [storedData, setStoredData] = useState([]); const updateStoredData = (audios, isCorrect) => { if (audios) { const newEntry = { + selectedAnswer: options && options.length > 0 && selectedOption !== null && selectedOption < options.length + ? options[selectedOption]?.text + : null, - selectedAnswer: - options && options.length > 0 && options[selectedOption]?.text, audioUrl: audios, correctAnswer: isCorrect, }; setStoredData((prevData) => [...prevData, newEntry]); } }; + +useEffect(() => { + return () => { + setSelectedOption(null); + setStoredData([]); + }; +}, []);Also applies to: 71-84
Line range hint
63-66: Improve audio handling and accessibilityThe audio implementation needs improvements in resource management and accessibility:
- Add cleanup for audio elements
- Handle audio loading errors
- Add ARIA labels for screen readers
const audiosRef = useRef( new Array(options.length).fill(null).map(() => React.createRef()) ); +const [audioLoadError, setAudioLoadError] = useState(false); useEffect(() => { // Ensure that audio stops playing when options change audiosRef.current.forEach((ref) => { if (ref.current && !ref.current.paused) { ref.current.pause(); } }); // Create new refs for the updated options audiosRef.current = new Array(options.length) .fill(null) .map(() => React.createRef()); setPlayingIndex(null); setSelectedOption(null); + setAudioLoadError(false); + + return () => { + audiosRef.current.forEach((ref) => { + if (ref.current) { + ref.current.pause(); + ref.current.src = ''; + } + }); + }; }, [options]);Add error handling and accessibility to audio elements:
<audio ref={questionAudioRef} preload="metadata" + aria-label="Question audio" onPlaying={() => setPlayingIndex("question")} onPause={() => setPlayingIndex(null)} + onError={() => setAudioLoadError(true)} > <source src={question_audio} type="audio/wav" /> + <track kind="captions" /> </audio>Also applies to: 85-108
Line range hint
186-262: Enhance accessibility and user experienceThe image zoom modal and selection interface need improvements in accessibility and error handling:
- Add keyboard navigation
- Handle image loading states
- Add error states for failed image loads
<Box sx={{ position: "relative", cursor: "zoom-in" }}> <img src={image} style={{ borderRadius: "20px", maxWidth: "100%", height: "250px", }} alt="contentImage" + onError={(e) => { + e.target.onerror = null; + e.target.src = 'fallback-image-url'; + }} + loading="lazy" onClick={() => setZoomOpen(true)} /> </Box> <Modal open={zoomOpen} onClose={() => setZoomOpen(false)} + onKeyDown={(e) => { + if (e.key === 'Escape') { + setZoomOpen(false); + } + }} + aria-labelledby="image-modal-title" + aria-describedby="image-modal-description" sx={{ display: "flex", alignItems: "center", justifyContent: "center", }} >src/utils/VoiceAnalyser.js (4)
Line range hint
133-137: Improve error handling in audio playback functionsThe error handling in both
playAudioandplayRecordedAudiofunctions contains duplicated code. Consider extracting the common error handling logic into a shared utility function.+const handleAudioError = (error, setPauseState) => { + console.error('Audio playback failed:', error); + setPauseState(false); + alert('Unable to play the audio. Please check your connection and try again.'); +}; -audio.addEventListener("error", (e) => { - console.error("Audio failed to load", e); - setPauseAudio(false); - alert("Failed to load the audio. Please try again."); -}); +audio.addEventListener("error", (e) => handleAudioError(e, setPauseAudio));Also applies to: 182-186
Line range hint
676-689: Enhance permission handling with specific error statesWhile the update to
mediaDevices.getUserMediais good, the error handling could be more specific about different permission states (denied, unavailable, etc.).-const getpermision = () => { +const getpermision = async () => { + try { navigator.mediaDevices .getUserMedia({ audio: true }) .then((stream) => { setAudioPermission(true); + // Clean up the stream when not needed + stream.getTracks().forEach(track => track.stop()); }) .catch((error) => { - console.log("Permission Denied"); + console.error("Microphone access error:", error.name); setAudioPermission(false); + switch(error.name) { + case 'NotAllowedError': + props.setOpenMessageDialog({ + message: "Microphone access was denied. Please enable it in your browser settings.", + isError: true + }); + break; + case 'NotFoundError': + props.setOpenMessageDialog({ + message: "No microphone device found. Please connect a microphone.", + isError: true + }); + break; + default: + props.setOpenMessageDialog({ + message: "Failed to access microphone. Please try again.", + isError: true + }); + } }); + } catch (error) { + console.error("Fatal error accessing media devices:", error); + setAudioPermission(false); + } };
Line range hint
315-476: RefactorfetchASROutputfor better separation of concernsThe function handles multiple responsibilities including audio processing, S3 upload, telemetry, and state management. This makes it hard to maintain and test.
Consider splitting it into smaller, focused functions:
- Audio processing
- S3 upload
- Telemetry
- State management
Example refactor for S3 upload:
+const uploadAudioToS3 = async (base64Data, sessionId, contentId) => { + if (process.env.REACT_APP_CAPTURE_AUDIO !== "true") return null; + + const audioFileName = `${process.env.REACT_APP_CHANNEL}/${sessionId}-${Date.now()}-${contentId}.wav`; + const command = new PutObjectCommand({ + Bucket: process.env.REACT_APP_AWS_S3_BUCKET_NAME, + Key: audioFileName, + Body: Uint8Array.from(window.atob(base64Data), (c) => c.charCodeAt(0)), + ContentType: "audio/wav", + }); + + try { + await S3Client.send(command); + return audioFileName; + } catch (err) { + console.error("Failed to upload audio:", err); + return null; + } +};
Line range hint
478-587: Extract lives calculation configuration and improve namingThe
handlePercentageForLifefunction contains hard-coded values and complex threshold calculations. Consider extracting the configuration and improving the function name.+const FLUENCY_THRESHOLDS = { + word: 2, + sentence: 6, + paragraph: 10 +}; + +const SYLLABLE_THRESHOLDS = [ + { max: 100, threshold: 30 }, + { max: 150, threshold: 25 }, + { max: 175, threshold: 20 }, + { max: 250, threshold: 15 }, + { max: 500, threshold: 10 }, + { max: Infinity, threshold: 5 } +]; + +const calculateThreshold = (totalSyllables) => { + const config = SYLLABLE_THRESHOLDS.find(t => totalSyllables <= t.max); + return config ? config.threshold : 5; +}; + -const handlePercentageForLife = ( +const updateLearnerLives = ( percentage, contentType, fluencyScore, language ) => {src/components/Assesment/Assesment.jsx (3)
Line range hint
1-842: Fix spelling: Rename component and file from "Assesment" to "Assessment"The word "Assessment" is consistently misspelled throughout the file. This affects code maintainability and searchability.
- Rename the file from
Assesment.jsxtoAssessment.jsx- Update the component name and all related imports/exports
- Update all references to this component in other files
570-570: Use 'const' for variables that aren't reassignedThe
userDetailsvariable is never reassigned, so it should useconstinstead oflet.- let userDetails = jwtDecode(jwtToken); + const userDetails = jwtDecode(jwtToken);
Line range hint
586-671: Consider refactoring for better maintainabilityThe useEffect hook contains complex logic with multiple API calls and state updates. Consider:
- Moving API calls to a separate service
- Creating custom hooks for state management
- Breaking down the component into smaller, more focused components
This will improve:
- Code maintainability
- Testing capabilities
- Reusability
Example service structure:
// services/assessmentService.js export const fetchUserDetails = async (username) => { const response = await axios.post( `${process.env.REACT_APP_VIRTUAL_ID_HOST}/${config.URLS.GET_VIRTUAL_ID}?username=${username}` ); return response.data; }; // hooks/useAssessment.js export const useAssessment = (username, lang) => { const [level, setLevel] = useState(""); const [points, setPoints] = useState(0); useEffect(() => { const fetchData = async () => { // API calls logic }; fetchData(); }, [username, lang]); return { level, points }; };src/components/Layouts.jsx/MainLayout.jsx (4)
Line range hint
155-160: Enhance error handling in audio playbackConsider replacing console.error with proper error handling that provides feedback to the user when audio fails to play.
if (!audioElem) { - console.error("Audio element not found:", audioElem); + // Show user-friendly error message + const errorMessage = "Unable to play audio at this time"; + props.onError?.(errorMessage); return; }
Line range hint
892-904: Strengthen PropTypes definitionsThe
handleNextprop is typed as 'any', which reduces type safety. Consider defining a more specific PropType.MainLayout.propTypes = { contentType: PropTypes.string, handleBack: PropTypes.func, disableScreen: PropTypes.bool, isShowCase: PropTypes.bool, showProgress: PropTypes.bool, setOpenLangModal: PropTypes.func, points: PropTypes.number, - handleNext: PropTypes.any, + handleNext: PropTypes.func.isRequired, enableNext: PropTypes.bool, showNext: PropTypes.bool, showTimer: PropTypes.bool, nextLessonAndHome: PropTypes.bool,
Line range hint
379-476: Optimize performance with style memoizationConsider extracting and memoizing commonly used styles to prevent unnecessary recalculations during re-renders.
+const useStyles = () => { + const commonBoxStyles = useMemo(() => ({ + display: "flex", + justifyContent: "center", + alignItems: "center", + }), []); + + return { commonBoxStyles }; +}; const MainLayout = (props) => { + const { commonBoxStyles } = useStyles(); // ... rest of the component
Line range hint
683-697: Enhance accessibility for audio controlsThe audio controls could benefit from improved accessibility features.
<button onClick={() => handleAudioPlay(index)} style={{ height: "30px", cursor: "pointer", background: "none", border: "none", padding: "0", }} + aria-label={audioPlaying === index ? "Pause audio" : "Play audio"} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + handleAudioPlay(index); + } + }} >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
src/components/Assesment/Assesment.jsx(8 hunks)src/components/AssesmentEnd/AssesmentEnd.jsx(3 hunks)src/components/DiscoverEnd/DiscoverEnd.jsx(4 hunks)src/components/Layouts.jsx/MainLayout.jsx(1 hunks)src/components/Practice/Mechanics1.jsx(1 hunks)src/components/Practice/Mechanics2.jsx(1 hunks)src/components/Practice/Mechanics3.jsx(1 hunks)src/components/Practice/Mechanics5.jsx(1 hunks)src/services/telementryService.js(2 hunks)src/utils/VoiceAnalyser.js(1 hunks)src/views/HomePage/HomePage.jsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/components/Practice/Mechanics1.jsx
- src/components/Practice/Mechanics3.jsx
🧰 Additional context used
🪛 Biome
src/services/telementryService.js
[error] 30-30: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (9)
src/views/HomePage/HomePage.jsx (1)
4-4: LGTM! Clean import statement.
The simplified import of useSearchParams directly from 'react-router-dom' follows best practices.
src/components/DiscoverEnd/DiscoverEnd.jsx (2)
4-5: LGTM! Import statements are well-organized.
The imports are correctly structured and all imported entities appear to be in use.
Also applies to: 11-12
137-137: LGTM! Event handler is properly optimized.
The onClick handler correctly uses the function reference instead of an arrow function wrapper.
src/components/AssesmentEnd/AssesmentEnd.jsx (3)
18-18: LGTM! Good practice using centralized URL constants.
Moving URL constants to a separate configuration file improves maintainability and makes it easier to manage environment-specific URLs.
42-44: LGTM! Improved code formatting.
The added spacing and semicolon improve code readability while maintaining the same functionality.
2-2:
Remove unused Typography import.
The Typography component is imported but not effectively used in the component. While there is one Typography component in the JSX, it could be replaced with a styled span to maintain consistency with other text elements in the component.
src/components/Practice/Mechanics5.jsx (1)
Line range hint 1-468: Verify integration with VoiceAnalyser component
Please ensure that the voice analysis integration works correctly with the new audio handling and selection state:
- Voice analysis should work with selected options
- Audio playback should coordinate properly between question, options, and voice recording
✅ Verification successful
The shell script results show that the VoiceAnalyser component is properly integrated with the necessary audio-related props and coordination mechanisms:
- The
audioLinkprop is correctly passed from Mechanics5 to VoiceAnalyser - Audio playback coordination is handled through refs and state management for question audio and options
- The VoiceAnalyser component properly handles the audio sources through the
audioLinkprop - Audio URLs are consistently handled across components with proper environment variable usage
Audio coordination and voice analysis integration verified successfully
The implementation correctly manages:
- Question audio playback through
questionAudioRef - Options audio through
audiosRefarray - Voice analysis integration with selected options
- Proper audio state management with
playingIndex
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for VoiceAnalyser usage and props
ast-grep --pattern 'VoiceAnalyser $props={$_}'
# Check for audio-related prop usage
rg -A 5 'audioLink|audio_url|question_audio'
Length of output: 8619
src/components/Assesment/Assesment.jsx (1)
431-438: LGTM: Image optimizations improve performance
The addition of width, height, and loading attributes helps prevent layout shifts and optimizes image loading performance.
Also applies to: 455-459
src/components/Layouts.jsx/MainLayout.jsx (1)
246-248: Good image optimization practices!
The addition of width, height, and lazy loading attributes helps prevent layout shifts and optimizes page load performance.
| import desktopLevel1 from "../../assets/images/desktopLevel1.png"; | ||
| import desktopLevel2 from "../../assets/images/desktopLevel2.png"; | ||
| import desktopLevel3 from "../../assets/images/desktopLevel3.jpg"; |
There was a problem hiding this comment.
💡 Codebase verification
Consider optimizing image assets, particularly desktopLevel3.jpg
The image files exist at the correct locations, but their sizes could be optimized:
desktopLevel3.jpgis notably large at 339KBdesktopLevel1.pnganddesktopLevel2.pngare moderately sized at 137KB and 125KB respectively
Consider:
- Converting PNG files to WebP format for better compression
- Optimizing the large JPEG file using image compression tools
- Using responsive image techniques with different sizes for various viewports
🔗 Analysis chain
Verify image assets existence and optimization.
Let's ensure all referenced image assets exist and are optimized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all referenced images exist and get their sizes
for img in "desktopLevel1.png" "desktopLevel2.png" "desktopLevel3.jpg"; do
find src/assets/images -name "$img" -exec ls -lh {} \;
done
Length of output: 686
| <img | ||
| src={back} | ||
| alt="back" | ||
| style={{ height: "30px" }} | ||
| width="30" // Set width attribute | ||
| height="30" // Set height attribute | ||
| loading="lazy" // Lazy-load the image | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply consistent image optimizations across all images.
While the back arrow image has been optimized with width, height, and lazy loading attributes, these optimizations should be applied consistently to all images in the component for better performance.
Consider removing the style height and using consistent attributes:
- <img
- src={back}
- alt="back"
- style={{ height: "30px" }}
- width="30"
- height="30"
- loading="lazy"
- />
+ <img
+ src={back}
+ alt="back"
+ width="30"
+ height="30"
+ loading="lazy"
+ />
// Apply similar optimizations to other images
- <img
- src={discoverEndLeft}
- alt="timer"
- className={shake && "shakeImage"}
- />
+ <img
+ src={discoverEndLeft}
+ alt="timer"
+ className={shake && "shakeImage"}
+ width="auto"
+ height="auto"
+ loading="lazy"
+ />Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (10)
src/components/Practice/Mechanics2.jsx (1)
21-28: Remove redundant commentThe comment "Cover the entire viewport" is redundant with the
backgroundSize: "cover"property name.backgroundImage: `url(${practicebg})`, - backgroundSize: "cover", // Cover the entire viewport + backgroundSize: "cover",src/utils/AudioCompare.js (2)
77-79: Simplify the null check implementation.The current implementation has redundant null checking. Since you're already checking if
props.setRecordedAudioexists with an if statement, the optional chaining operator (?.) is unnecessary.if (props.setRecordedAudio) { - props?.setRecordedAudio(url); + props.setRecordedAudio(url); }
Line range hint
1-209: Consider improving error handling and accessibility.
- Error Handling: The component silently logs errors to console when recording fails. Consider adding error state and user feedback.
- Accessibility: Add appropriate ARIA labels and roles for audio controls.
Consider these improvements:
- Add error state and user feedback:
const [error, setError] = useState(null); // In catch block: setError("Failed to access microphone. Please check permissions.");
- Enhance accessibility:
<Box sx={{ cursor: "pointer" }} onClick={startRecording} + role="button" + aria-label="Start recording" + tabIndex={0} >src/components/DiscoverSentance/DiscoverSentance.jsx (3)
Line range hint
89-101: Consider enhancing error handling for voice recognitionThe current error handling could be improved in several ways:
- Add more specific error messages based on different error scenarios
- Implement a debounce mechanism to prevent rapid state changes
- Add error logging for debugging purposes
useEffect(() => { if (voiceText === "error") { - setOpenMessageDialog({ - message: "Sorry I couldn't hear a voice. Could you please speak again?", - isError: true, - }); - setVoiceText(""); - setEnableNext(false); + const handleVoiceError = () => { + // Add error logging + console.error('Voice recognition error occurred'); + + setOpenMessageDialog({ + message: "Sorry I couldn't hear a voice. Could you please speak again?", + errorCode: "VOICE_RECOGNITION_ERROR", + isError: true, + }); + + // Use setTimeout to prevent race conditions + setTimeout(() => { + setVoiceText(""); + setEnableNext(false); + }, 100); + }; + + handleVoiceError(); } if (voiceText == "success") { setVoiceText(""); } }, [voiceText]);
Line range hint
116-238: Consider refactoring handleNext for better maintainabilityThe
handleNextfunction is handling too many responsibilities, making it hard to maintain and test. Consider breaking it down into smaller, focused functions.
- Extract API calls into separate service functions
- Split the navigation logic into a separate function
- Implement proper error handling for API calls
Example refactor:
// API service functions const updatePoints = async (lang) => { if (localStorage.getItem("contentSessionId") === null) { const response = await axios.post( `${process.env.REACT_APP_LEARNER_AI_ORCHESTRATION_HOST}/${config.URLS.ADD_POINTER}`, { userId: localStorage.getItem("virtualId"), sessionId: localStorage.getItem("sessionId"), points: 1, language: lang, milestone: "m0", } ); return response?.data?.result?.totalLanguagePoints || 0; } send(1); return null; }; // Navigation handler const handleNavigation = async (sessionResult, currentContentType) => { if (sessionResult === "pass" && currentContentType === "Sentence") { // Handle sentence navigation } else if (sessionResult === "pass") { navigate("/discover-end"); } // ... rest of navigation logic }; // Refactored handleNext const handleNext = async () => { try { setIsNextButtonCalled(true); setEnableNext(false); const lang = getLocalData("lang"); const points = await updatePoints(lang); if (points !== null) { setPoints(points); } // ... rest of the logic broken down into smaller functions } catch (error) { console.error('Error in handleNext:', error); setOpenMessageDialog({ message: "An error occurred. Please try again.", isError: true }); } };
1-1: Add PropTypes for better type safetyTo maintain consistency with other components and improve type safety, consider adding PropTypes to the component.
import PropTypes from 'prop-types'; // ... component code ... SpeakSentenceComponent.propTypes = { // Add any props if component starts accepting them in the future }; export default SpeakSentenceComponent;src/components/Practice/Mechanics5.jsx (4)
Line range hint
71-71: Prevent potential null reference errorsThe component attempts to access
options[selectedOption]whenselectedOptioncould be null. This could lead to runtime errors.- selectedOption={options[selectedOption]} + selectedOption={selectedOption !== null ? options[selectedOption] : null}Also applies to: 365-365
Line range hint
89-93: Fix incorrect useEffect for updateStoredDataThe useEffect calls updateStoredData without parameters, but the function expects audios and isCorrect parameters. This will result in undefined values being stored.
useEffect(() => { - updateStoredData(); + if (selectedOption !== null && options[selectedOption]) { + updateStoredData(null, correctness); + } }, [handleNext]);Also applies to: 97-99
Line range hint
100-111: Optimize audio loading performancePreloading all audio files simultaneously could impact initial load performance, especially with many options.
<audio ref={audiosRef.current[i]} - preload="metadata" + preload="none" onPlaying={() => setPlayingIndex(i)} onPause={() => setPlayingIndex(null)} >Consider loading audio metadata only when the user interacts with the play button.
Line range hint
332-336: Implement secure URL handling for audio sourcesDirect concatenation of URLs without proper encoding could lead to security issues.
+ const getSecureAudioUrl = (audioUrl) => { + const encodedUrl = encodeURIComponent(audioUrl); + return `${process.env.REACT_APP_AWS_S3_BUCKET_CONTENT_URL}/mechanics_audios/${encodedUrl}`; + }; <source - src={`${process.env.REACT_APP_AWS_S3_BUCKET_CONTENT_URL}/mechanics_audios/${option.audio_url}`} + src={getSecureAudioUrl(option.audio_url)} type="audio/wav" />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
src/components/Assesment/Assesment.jsx(8 hunks)src/components/DiscoverSentance/DiscoverSentance.jsx(3 hunks)src/components/Practice/Mechanics1.jsx(1 hunks)src/components/Practice/Mechanics2.jsx(1 hunks)src/components/Practice/Mechanics5.jsx(1 hunks)src/utils/AudioCompare.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Practice/Mechanics1.jsx
🔇 Additional comments (12)
src/components/Practice/Mechanics2.jsx (3)
1-18: LGTM! Well-organized imports
The imports are properly structured with external dependencies first, followed by internal assets.
35-42: Existing issue still applies
The redundant height definition issue identified in the previous review still exists.
98-98: Add page limit validation
The onClick handler should validate against a maximum page limit to prevent invalid state.
- onClick={() => setPage(page + 1)}
+ onClick={() => {
+ const MAX_PAGES = 5; // adjust based on your needs
+ if (page < MAX_PAGES) {
+ setPage(page + 1);
+ }
+ }}src/utils/AudioCompare.js (1)
8-8: LGTM! Good addition of PropTypes.
Adding PropTypes improves type safety and documentation of the component's interface.
src/components/DiscoverSentance/DiscoverSentance.jsx (2)
3-3: LGTM! Import statement improvement
The change to use a direct import for axios follows best practices for third-party packages.
65-65: LGTM! Improved condition readability
The explicit null check localStorage.getItem("contentSessionId") === null is clearer than the previous negation pattern. This change maintains consistency across the codebase and improves code readability.
Also applies to: 120-120
src/components/Assesment/Assesment.jsx (6)
10-10: LGTM: Import statements are properly organized.
The imports are well-organized and grouped logically:
- Material-UI components
- React and routing utilities
- Third-party libraries
- Local assets and utilities
Also applies to: 24-24, 25-25, 29-29
54-58: LGTM: PropTypes are properly defined for LanguageModal.
The PropTypes are correctly defined with appropriate types and required flags.
441-448: LGTM: Image attributes are properly optimized.
The back button image includes proper width, height, and loading attributes for better performance and accessibility.
465-469: LGTM: Profile picture image attributes are properly optimized.
The profile picture image includes proper width, height, and loading attributes for better performance and accessibility.
568-574: LGTM: PropTypes are properly defined for ProfileHeader.
The PropTypes are correctly defined with appropriate types.
933-935: LGTM: PropTypes are properly defined for Assesment component.
The PropTypes are correctly defined for the main component.
| matchedChar: PropTypes.any, | ||
| question_audio: PropTypes.string.isRequired, | ||
| parentWords: PropTypes.string.isRequired, | ||
| options: PropTypes.any, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define specific PropTypes for options array
The options prop is currently typed as any which is too permissive. Based on the component's usage, we should define a more specific PropType structure.
- options: PropTypes.any,
+ options: PropTypes.arrayOf(
+ PropTypes.shape({
+ text: PropTypes.string.isRequired,
+ audio_url: PropTypes.string.isRequired,
+ isAns: PropTypes.bool.isRequired
+ })
+ ).isRequired,📝 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.
| options: PropTypes.any, | |
| options: PropTypes.arrayOf( | |
| PropTypes.shape({ | |
| text: PropTypes.string.isRequired, | |
| audio_url: PropTypes.string.isRequired, | |
| isAns: PropTypes.bool.isRequired | |
| }) | |
| ).isRequired, |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (12)
src/utils/AudioCompare.js (2)
77-79: Improve error handling and remove redundant optional chaining.The conditional check is good, but the code can be improved:
- Remove redundant optional chaining since we're already inside an if-check
- Add error handling for URL creation
- if (props.setRecordedAudio) { - props?.setRecordedAudio(url); - } + try { + if (props.setRecordedAudio) { + props.setRecordedAudio(url); + } + } catch (error) { + console.error('Failed to create audio URL:', error); + } finally { + // Clean up the URL to prevent memory leaks + URL.revokeObjectURL(url); + }
Line range hint
10-209: Consider extracting audio recording logic into a custom hook.The component mixes UI rendering with complex audio recording logic. Consider creating a custom hook (e.g.,
useAudioRecorder) to:
- Handle all recording-related state and logic
- Manage media streams and cleanup
- Provide a cleaner API for the component
This would improve:
- Code organization
- Reusability
- Testability
- Maintenance
Example structure:
// useAudioRecorder.js const useAudioRecorder = (options) => { // Move all recording logic here return { startRecording, stopRecording, status, // ... other necessary values }; }; // AudioRecorder.js const AudioRecorder = (props) => { const { startRecording, stopRecording, status } = useAudioRecorder({ onRecordingComplete: props.setRecordedAudio, // ... other options }); // Component now focuses on rendering UI return ( // ... JSX ); };src/services/telementryService.js (4)
30-30: Improve URL parsing with optional chainingThe current URL parsing could be simplified and made safer using optional chaining.
-url = getUrl && getUrl.includes("#") && getUrl.split("#")[1].split("/")[1]; +url = getUrl?.split("#")?.[1]?.split("/")?.[1] ?? "";🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
60-64: Improve error logging in initialize functionThe current error logging is not descriptive enough for debugging purposes.
try { await CsTelemetryModule.instance.telemetryService.initTelemetry( telemetryConfig ); } catch (error) { - console.log(":e", error); + console.error("Failed to initialize telemetry:", error); }
77-77: Replace magic number with named constantThe use of magic number
1e3makes the code less readable.- duration: Number((duration / 1e3).toFixed(2)), + const MILLISECONDS_TO_SECONDS = 1000; + duration: Number((duration / MILLISECONDS_TO_SECONDS).toFixed(2)),
198-204: Simplify telemetry mode checking logicThe current implementation of telemetry mode checking is complex and could be simplified.
- return ( - (process.env.REACT_APP_TELEMETRY_MODE === "ET" && currentMode === "ET") || - (process.env.REACT_APP_TELEMETRY_MODE === "NT" && - (currentMode === "ET" || currentMode === "NT")) || - (process.env.REACT_APP_TELEMETRY_MODE === "DT" && - (currentMode === "ET" || currentMode === "NT" || currentMode === "DT")) - ); + const modeHierarchy = { + 'ET': ['ET'], + 'NT': ['ET', 'NT'], + 'DT': ['ET', 'NT', 'DT'] + }; + const allowedModes = modeHierarchy[process.env.REACT_APP_TELEMETRY_MODE] || []; + return allowedModes.includes(currentMode);src/utils/VoiceAnalyser.js (2)
Line range hint
89-116: Consider centralizing error handling logicThe error handling in
playAudioandplayRecordedAudiofunctions is similar but duplicated. Consider extracting the common error handling logic into a shared utility function.+const handleAudioError = (error, setStateFunction) => { + console.error('Audio playback failed:', error); + setStateFunction(false); + props.setOpenMessageDialog({ + message: 'Failed to load the audio. Please try again.', + isError: true + }); +}; const playAudio = async (val) => { if (isStudentAudioPlaying) { return; } const { audioLink } = props; try { // ... existing code ... - audio.addEventListener("error", (e) => { - console.error("Audio failed to load", e); - setPauseAudio(false); - alert("Failed to load the audio. Please try again."); - }); + audio.addEventListener("error", (e) => handleAudioError(e, setPauseAudio)); } catch (err) { - console.error("An error occurred:", err); - alert("An unexpected error occurred while trying to play the audio."); + handleAudioError(err, setPauseAudio); } };Also applies to: 146-170
Line range hint
71-85: Add missing dependencies to useEffectThe
useEffecthook that watchesprops.enableNextshould include all dependencies it uses.useEffect(() => { if (!props.enableNext) { setRecordedAudio(""); } -}, [props.enableNext]); +}, [props.enableNext, setRecordedAudio]);src/components/Assesment/Assesment.jsx (1)
Line range hint
12-24: Consider revising the discount calculation logic.The current implementation adds a flat $20 fee to discounted amounts, which could negate the benefit of smaller discounts. For example, a $100 purchase with 10% discount would cost $110 after the fee, more than the original price.
Consider one of these alternatives:
- Apply the fee before the discount
- Scale the fee based on the purchase amount
- Remove the fee for smaller purchases
Would you like me to provide an implementation for any of these alternatives?
src/views/Practice/Practice.jsx (3)
125-146: Refactor duplicate origin validation logic into a reusable functionThe origin validation and message posting logic is duplicated in multiple places within this file (lines 125-146 and lines 778-797). Refactoring this code into a reusable utility function will improve maintainability and reduce redundancy.
You can create a utility function called
postMessageToParentto handle this logic:+// Utility function for posting messages to parent with origin validation +const postMessageToParent = (message) => { + let allowedOrigins = []; + try { + allowedOrigins = JSON.parse(process.env.REACT_APP_PARENT_ORIGIN_URL || "[]"); + } catch (error) { + console.error("Invalid JSON format in REACT_APP_PARENT_ORIGIN_URL:", error); + } + const parentOrigin = window?.location?.ancestorOrigins?.[0] || window.parent.location.origin; + if (allowedOrigins.includes(parentOrigin)) { + window.parent.postMessage(message, parentOrigin); + } +}; -const send = (score) => { - if (process.env.REACT_APP_IS_APP_IFRAME === "true") { - let allowedOrigins = []; - try { - allowedOrigins = JSON.parse( - process.env.REACT_APP_PARENT_ORIGIN_URL || "[]" - ); - } catch (error) { - console.error( - "Invalid JSON format in REACT_APP_PARENT_ORIGIN_URL:", - error - ); - } - const parentOrigin = - window?.location?.ancestorOrigins?.[0] || window.parent.location.origin; - if (allowedOrigins.includes(parentOrigin)) { - window.parent.postMessage( - { - score: score, - message: "all-test-rig-score", - }, - parentOrigin - ); - } - } -}; +const send = (score) => { + if (process.env.REACT_APP_IS_APP_IFRAME === "true") { + postMessageToParent({ + score: score, + message: "all-test-rig-score", + }); + } +};
778-797: Refactor duplicate origin validation logic into a reusable functionAs with the previous instance, the code for origin validation and message posting is duplicated here. Utilize the
postMessageToParentutility function to avoid redundancy.Apply this change to use the utility function:
-if (process.env.REACT_APP_IS_APP_IFRAME === "true") { - const contentSourceData = - questions[currentQuestion]?.contentSourceData || []; - const stringLengths = contentSourceData.map((item) => item.text.length); - const length = - questions[currentQuestion]?.mechanics_data && - (questions[currentQuestion]?.mechanics_data[0]?.mechanics_id === - "mechanic_2" || - questions[currentQuestion]?.mechanics_data[0]?.mechanics_id === - "mechanic_1") - ? 500 - : stringLengths[0]; - let allowedOrigins = []; - try { - allowedOrigins = JSON.parse( - process.env.REACT_APP_PARENT_ORIGIN_URL || "[]" - ); - } catch (error) { - console.error( - "Invalid JSON format in REACT_APP_PARENT_ORIGIN_URL:", - error - ); - } - const parentOrigin = - window?.location?.ancestorOrigins?.[0] || - window.parent.location.origin; - if (allowedOrigins.includes(parentOrigin)) { - window.parent.postMessage( - { type: "stringLengths", length }, - parentOrigin - ); - } -} +if (process.env.REACT_APP_IS_APP_IFRAME === "true") { + const contentSourceData = + questions[currentQuestion]?.contentSourceData || []; + const stringLengths = contentSourceData.map((item) => item.text.length); + const length = + questions[currentQuestion]?.mechanics_data && + (questions[currentQuestion]?.mechanics_data[0]?.mechanics_id === + "mechanic_2" || + questions[currentQuestion]?.mechanics_data[0]?.mechanics_id === + "mechanic_1") + ? 500 + : stringLengths[0]; + postMessageToParent({ type: "stringLengths", length }); +}
131-134: Avoid logging sensitive information in error messagesThe error logs in the
catchblocks may potentially expose sensitive information. Consider logging only the error message without the error object or use a less verbose logging level in production.Apply this change to reduce verbosity:
console.error( "Invalid JSON format in REACT_APP_PARENT_ORIGIN_URL:", - error );Also applies to: 784-787
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
src/components/Assesment/Assesment.jsx(9 hunks)src/components/DiscoverSentance/DiscoverSentance.jsx(4 hunks)src/components/Layouts.jsx/MainLayout.jsx(1 hunks)src/services/telementryService.js(2 hunks)src/utils/AudioCompare.js(3 hunks)src/utils/VoiceAnalyser.js(2 hunks)src/views/Practice/Practice.jsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/DiscoverSentance/DiscoverSentance.jsx
- src/components/Layouts.jsx/MainLayout.jsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/services/telementryService.js
[error] 30-30: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (8)
src/utils/AudioCompare.js (2)
8-8: LGTM!
Clean PropTypes import following React best practices.
196-208: Previous PropTypes enhancement comment is still applicable.
The existing review comment about enhancing PropTypes with required props and defaults is still valid and should be addressed.
src/utils/VoiceAnalyser.js (2)
750-750: LGTM: Required prop type for originalText
The change to make originalText a required prop improves type safety and ensures the component receives necessary data.
764-764:
Maintain type safety for updateStoredData prop
The change from PropTypes.func.isRequired to PropTypes.any weakens type safety. Since this prop is used as a function in fetchASROutput, it should maintain its function type definition.
- updateStoredData: PropTypes.any,
+ updateStoredData: PropTypes.func,Likely invalid or redundant comment.
src/components/Assesment/Assesment.jsx (4)
55-59: LGTM! PropTypes implementation improves type safety.
The addition of PropTypes for all components enhances code reliability and maintainability by providing runtime type checking.
Also applies to: 432-437, 733-739, 1090-1092
552-559: Image optimizations improve performance.
Good additions of width, height, and loading attributes to prevent layout shifts and optimize loading performance.
Also applies to: 576-580
745-745: LGTM! Using const improves code safety.
Good practice to use const instead of let since the variable is not reassigned.
455-474: Enhanced security with origin validation and error handling.
Good implementation of:
- Parsing and validating allowed origins
- Proper error handling for JSON parsing
- Fallback for ancestor origins
Let's verify the allowed origins configuration:
✅ Verification successful
Security implementation is consistent across the codebase
The origin validation pattern is consistently implemented across all files that use postMessage communication:
- All instances properly parse and validate allowed origins from REACT_APP_PARENT_ORIGIN_URL
- Each implementation includes proper error handling for JSON parsing
- The same secure fallback mechanism for ancestor origins is used
- Origin validation is performed before sending any messages
The reviewed code follows the same secure pattern used throughout the application, maintaining a robust cross-origin security model.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if REACT_APP_PARENT_ORIGIN_URL is properly configured in environment files
fd -e env -e env.development -e env.production --exec grep -l "REACT_APP_PARENT_ORIGIN_URL"
Length of output: 91
Script:
#!/bin/bash
# Let's check for any environment configuration files in the repository
fd -e env -e env.example -e env.development -e env.production
# Also search for any documentation or configuration mentioning this environment variable
rg "REACT_APP_PARENT_ORIGIN_URL" -l
Length of output: 223
Script:
#!/bin/bash
# Let's examine how this environment variable is used in other files
rg "REACT_APP_PARENT_ORIGIN_URL" -A 5 -B 5
# Also check for any postMessage usage in these files to ensure consistent security practices
ast-grep --pattern 'postMessage($$$)'
Length of output: 5335
| { id: userDetails?.school_name, type: "school_name" }, | ||
| { | ||
| id: userDetails?.class_studying_id, | ||
| type: "class_studying_id", | ||
| }, | ||
| { id: userDetails?.udise_code, type: "udise_code" }, | ||
| ], |
There was a problem hiding this comment.
Add null checks for userDetails access
The code accesses properties of userDetails without checking if it exists, which could lead to runtime errors.
- { id: userDetails?.school_name, type: "school_name" },
- {
- id: userDetails?.class_studying_id,
- type: "class_studying_id",
- },
- { id: userDetails?.udise_code, type: "udise_code" },
+ ...(userDetails ? [
+ { id: userDetails.school_name, type: "school_name" },
+ { id: userDetails.class_studying_id, type: "class_studying_id" },
+ { id: userDetails.udise_code, type: "udise_code" }
+ ] : []),📝 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.
| { id: userDetails?.school_name, type: "school_name" }, | |
| { | |
| id: userDetails?.class_studying_id, | |
| type: "class_studying_id", | |
| }, | |
| { id: userDetails?.udise_code, type: "udise_code" }, | |
| ], | |
| ...(userDetails ? [ | |
| { id: userDetails.school_name, type: "school_name" }, | |
| { id: userDetails.class_studying_id, type: "class_studying_id" }, | |
| { id: userDetails.udise_code, type: "udise_code" } | |
| ] : []), |
| if (localStorage.getItem("token") !== null) { | ||
| let jwtToken = localStorage.getItem("token"); | ||
| const userDetails = jwtDecode(jwtToken); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant JWT decoding
The JWT token is being decoded but the decoded value isn't being used, making this operation unnecessary.
-if (localStorage.getItem("token") !== null) {
- let jwtToken = localStorage.getItem("token");
- const userDetails = jwtDecode(jwtToken);
-}Committable suggestion skipped: line range outside the PR's diff.
| let allowedOrigins = []; | ||
| try { | ||
| allowedOrigins = JSON.parse( | ||
| process.env.REACT_APP_PARENT_ORIGIN_URL || "[]" | ||
| ); | ||
| } catch (error) { | ||
| console.error( | ||
| "Invalid JSON format in REACT_APP_PARENT_ORIGIN_URL:", | ||
| error | ||
| ); | ||
| } | ||
| const parentOrigin = | ||
| window?.location?.ancestorOrigins?.[0] || window.parent.location.origin; | ||
| if (allowedOrigins.includes(parentOrigin)) { | ||
| window.parent.postMessage( | ||
| { | ||
| score: score, | ||
| message: "all-test-rig-score", | ||
| }, | ||
| parentOrigin | ||
| ); | ||
| } |
There was a problem hiding this comment.
Ensure robustness of 'parentOrigin' retrieval and handling
The retrieval of parentOrigin may fail or return undefined in certain cross-origin scenarios, which could lead to unexpected behavior when calling window.parent.postMessage. Consider adding a validation check for parentOrigin before using it.
Apply this change to handle cases where parentOrigin is undefined:
const parentOrigin =
window?.location?.ancestorOrigins?.[0] || window.parent.location.origin;
+if (!parentOrigin) {
+ console.error("Unable to determine parent origin.");
+ return;
+}
if (allowedOrigins.includes(parentOrigin)) {
window.parent.postMessage(message, parentOrigin);
}Also applies to: 778-797
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (12)
src/components/Assesment/AudioDiagnosticTool.jsx (1)
438-447: Enhance button accessibility.While changing to a
buttonelement improves semantics, consider these accessibility enhancements:<button + aria-label="Start recording" onClick={startRecording} - onKeyDown={(e) => { - if (e.key === "Enter") { - startRecording(); - } - }} >Note: The
onKeyDownhandler can be removed asbuttonelements already handle Enter key by default.src/components/Assesment/Assesment.jsx (5)
47-51: Enhance PropTypes definitions with required props and specific types.While PropTypes have been added, some essential props should be marked as required. Consider the following improvements:
TestModal.propTypes = { - setOpenTestModal: PropTypes.func, + setOpenTestModal: PropTypes.func.isRequired, }; MessageDialog.propTypes = { - closeDialog: PropTypes.func, + closeDialog: PropTypes.func.isRequired, - message: PropTypes.string, + message: PropTypes.string.isRequired, dontShowHeader: PropTypes.bool, isError: PropTypes.bool, }; ProfileHeader.propTypes = { points: PropTypes.number, - setOpenLangModal: PropTypes.func, + setOpenLangModal: PropTypes.func.isRequired, handleBack: PropTypes.func, - profileName: PropTypes.string, + profileName: PropTypes.string.isRequired, - lang: PropTypes.string, + lang: PropTypes.string.isRequired, setOpenTestModal: PropTypes.func, };Also applies to: 306-308, 427-432, 728-735, 1086-1088
450-469: Improve error handling in parent window communication.While the error handling has been improved, consider these enhancements:
try { allowedOrigins = JSON.parse( process.env.REACT_APP_PARENT_ORIGIN_URL || "[]" ); } catch (error) { console.error( - "Invalid JSON format in REACT_APP_PARENT_ORIGIN_URL:", + "Invalid JSON format in REACT_APP_PARENT_ORIGIN_URL. Expected array format:", error ); + allowedOrigins = []; } const parentOrigin = window?.location?.ancestorOrigins?.[0] || window.parent.location.origin; +if (!parentOrigin) { + console.error("Unable to determine parent origin"); + return; +} if (allowedOrigins.includes(parentOrigin)) { window.parent.postMessage( { type: "restore-iframe-content" }, parentOrigin ); +} else { + console.warn(`Parent origin "${parentOrigin}" is not in allowed list:`, allowedOrigins); }
Line range hint
873-889: Enhance user validation in handleRedirect function.The current validation could be more informative and maintainable.
const handleRedirect = () => { const profileName = getLocalData("profileName"); - if (!username && !profileName && !virtualId && level === 0) { - setOpenMessageDialog({ - message: "please add username in query param", - isError: true, - }); - return; - } + const missingFields = []; + if (!username) missingFields.push("username"); + if (!profileName) missingFields.push("profile name"); + if (!virtualId) missingFields.push("virtual ID"); + + if (missingFields.length > 0 && level === 0) { + setOpenMessageDialog({ + message: `Missing required fields: ${missingFields.join(", ")}`, + isError: true, + }); + return; + } if (level === 0) { navigate("/discover"); } else { navigate("/practice"); } };
Line range hint
763-845: Refactor useEffect to improve maintainability and error handling.The current implementation has complex nested logic and lacks proper error handling for API calls.
Consider extracting the API calls into separate functions and adding proper error handling:
+const fetchVirtualId = async (username) => { + try { + const response = await axios.post( + `${process.env.REACT_APP_VIRTUAL_ID_HOST}/${config.URLS.GET_VIRTUAL_ID}?username=${username}` + ); + return response?.data?.result?.virtualID; + } catch (error) { + console.error("Error fetching virtual ID:", error); + throw error; + } +}; + +const fetchMilestoneDetails = async (virtualId, language) => { + try { + const response = await axios.get( + `${process.env.REACT_APP_LEARNER_AI_APP_HOST}/${config.URLS.GET_MILESTONE}/${virtualId}?language=${language}` + ); + return response.data; + } catch (error) { + console.error("Error fetching milestone details:", error); + throw error; + } +}; useEffect(() => { const initializeUser = async () => { try { if (discoverStart && username && !localStorage.getItem("virtualId")) { - setLocalData("profileName", username); - const usernameDetails = await axios.post(...); - const getMilestoneDetails = await axios.get(...); + const virtualId = await fetchVirtualId(username); + const milestoneDetails = await fetchMilestoneDetails(virtualId, lang); - localStorage.setItem("getMilestone", JSON.stringify({ ...getMilestoneDetails.data })); + setLocalData("profileName", username); + localStorage.setItem("getMilestone", JSON.stringify(milestoneDetails)); + localStorage.setItem("virtualId", virtualId); - setLevel(getMilestoneDetails?.data.data?.milestone_level?.replace("m", "")); + setLevel(milestoneDetails?.data?.milestone_level?.replace("m", "")); // ... rest of the code + } + } catch (error) { + console.error("Error initializing user:", error); + setOpenMessageDialog({ + message: "Failed to initialize user data. Please try again.", + isError: true + }); } }; initializeUser(); }, [lang]);
Line range hint
644-676: Enhance accessibility of the help button.While keyboard navigation has been added, the button could be more accessible.
<button style={{...}} + aria-label="Help" + role="button" tabIndex="0" onKeyDown={(e) => { - if (e.key === "Enter") { + if (e.key === "Enter" || e.key === "Space") { + e.preventDefault(); e.target.style.transform = "scale(1)"; + setOpenTestModal?.(true); } }} + onClick={() => setOpenTestModal?.(true)} > <span style={{ fontWeight: "bold", marginBottom: "2px" }}> ? </span> </button>src/views/Practice/Practice.jsx (5)
124-145: Consider extracting message posting logic into a utility functionThe origin validation and message posting logic is duplicated in the codebase. Consider extracting it into a reusable utility function to improve maintainability and reduce code duplication.
+const postMessageToParent = (message, allowedOrigins) => { + try { + const origins = JSON.parse(allowedOrigins || "[]"); + const parentOrigin = window?.location?.ancestorOrigins?.[0] || window.parent.location.origin; + if (origins.includes(parentOrigin)) { + window.parent.postMessage(message, parentOrigin); + } + } catch (error) { + console.error("Error posting message to parent:", error); + } +}; const send = (score) => { if (process.env.REACT_APP_IS_APP_IFRAME === "true") { - let allowedOrigins = []; - try { - allowedOrigins = JSON.parse(process.env.REACT_APP_PARENT_ORIGIN_URL || "[]"); - } catch (error) { - console.error("Invalid JSON format in REACT_APP_PARENT_ORIGIN_URL:", error); - } - const parentOrigin = window?.location?.ancestorOrigins?.[0] || window.parent.location.origin; - if (allowedOrigins.includes(parentOrigin)) { - window.parent.postMessage( - { score: score, message: "all-test-rig-score" }, - parentOrigin - ); - } + postMessageToParent( + { score: score, message: "all-test-rig-score" }, + process.env.REACT_APP_PARENT_ORIGIN_URL + ); } };
216-216: Use constants for milestone valuesUsing string literals for milestone values ('showcase', 'practice') could lead to maintenance issues. Consider defining these as constants.
+const MILESTONE_TYPES = { + SHOWCASE: 'showcase', + PRACTICE: 'practice' +}; -milestone: isShowCase ? "showcase" : "practice", +milestone: isShowCase ? MILESTONE_TYPES.SHOWCASE : MILESTONE_TYPES.PRACTICE,
Line range hint
722-752: Use Fragment shorthand syntaxConsider using the shorthand syntax
<>...</>instead of<React.Fragment>for cleaner code when key prop is not needed.-<React.Fragment key={word}> +<Fragment key={word}> <Typography variant="h5" component="h4" ml={1} sx={{...}} > {word} </Typography>{" "} -</React.Fragment> +</Fragment>
Line range hint
807-824: Add PropTypes validationConsider adding PropTypes validation for the components receiving these props to improve type safety and documentation.
import PropTypes from 'prop-types'; WordsOrImage.propTypes = { level: PropTypes.number, header: PropTypes.string.isRequired, words: PropTypes.string, contentType: PropTypes.string.isRequired, // ... add other props }; Mechanics3.propTypes = { level: PropTypes.number, header: PropTypes.string.isRequired, parentWords: PropTypes.string, // ... add other props };Also applies to: 854-857, 867-868, 876-876, 883-886
1083-1138: Add error handling to utility functionsThe utility functions could benefit from error handling to gracefully handle undefined or malformed input data.
const getImageUrl = (mechanicsData) => { + if (!mechanicsData?.[0]?.image_url) { + console.warn('Missing image URL in mechanics data'); + return null; + } return mechanicsData ? `${process.env.REACT_APP_AWS_S3_BUCKET_CONTENT_URL}/mechanics_images/${mechanicsData[0]?.image_url}` : null; };src/components/Layouts.jsx/MainLayout.jsx (1)
Line range hint
36-38: Consider adding error handling for audio playback.The audio playback functionality should handle potential errors that might occur during playback.
Consider adding error handling:
if (audioElem.paused) { - audioElem.play(); + audioElem.play().catch(error => { + console.error('Error playing audio:', error); + setAudioPlaying(null); + }); setAudioPlaying(index); audioElem.onended = () => { setAudioPlaying(null); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/components/Assesment/Assesment.jsx(13 hunks)src/components/Assesment/AudioDiagnosticTool.jsx(2 hunks)src/components/Layouts.jsx/MainLayout.jsx(5 hunks)src/views/Practice/Practice.jsx(21 hunks)
🔇 Additional comments (6)
src/components/Assesment/AudioDiagnosticTool.jsx (3)
7-7: LGTM! Good addition of PropTypes for type checking.
Adding PropTypes improves code reliability by enabling runtime type checking for props.
426-435: LGTM! Comprehensive PropTypes validation for TestSection.
All props are properly typed and marked as required, which aligns with their usage in the component.
450-453: LGTM! Proper PropTypes validation for RecordingButton.
Props are correctly typed and marked as required, matching their usage in the component.
src/views/Practice/Practice.jsx (1)
777-796: Reuse the postMessageToParent utility function
This code block has the same origin validation logic as the send function. Reuse the suggested utility function here as well.
-let allowedOrigins = [];
-try {
- allowedOrigins = JSON.parse(process.env.REACT_APP_PARENT_ORIGIN_URL || "[]");
-} catch (error) {
- console.error("Invalid JSON format in REACT_APP_PARENT_ORIGIN_URL:", error);
-}
-const parentOrigin = window?.location?.ancestorOrigins?.[0] || window.parent.location.origin;
-if (allowedOrigins.includes(parentOrigin)) {
- window.parent.postMessage({ type: "stringLengths", length }, parentOrigin);
-}
+postMessageToParent(
+ { type: "stringLengths", length },
+ process.env.REACT_APP_PARENT_ORIGIN_URL
+);src/components/Layouts.jsx/MainLayout.jsx (2)
251-253: Good addition of image attributes for optimization!
The addition of width, height, and lazy loading attributes helps improve page performance and prevents layout shifts.
359-359: Good addition of key props for list rendering!
Adding unique key props to mapped elements helps React efficiently update the DOM and eliminates console warnings.
Also applies to: 384-384, 391-391, 833-833
| level: PropTypes.number, | ||
| handleNext: PropTypes.func, | ||
| enableNext: PropTypes.bool, | ||
| showNext: PropTypes.bool, | ||
| showTimer: PropTypes.bool, | ||
| nextLessonAndHome: PropTypes.bool, | ||
| cardBackground: PropTypes.string, | ||
| backgroundImage: PropTypes.string, | ||
| points: PropTypes.number, | ||
| progressData: PropTypes.shape({ | ||
| currentPracticeStep: PropTypes.number, | ||
| currentPracticeProgress: PropTypes.number, | ||
| }), | ||
| showProgress: PropTypes.bool, | ||
| setOpenLangModal: PropTypes.func, | ||
| setOpenTestModal: PropTypes.func, | ||
| lang: PropTypes.string, | ||
| handleBack: PropTypes.func, | ||
| disableScreen: PropTypes.bool, | ||
| isShowCase: PropTypes.bool, | ||
| startShowCase: PropTypes.bool, | ||
| contentType: PropTypes.string, | ||
| percentage: PropTypes.number, | ||
| fluency: PropTypes.bool, | ||
| setStartShowCase: PropTypes.func, | ||
| livesData: PropTypes.shape({ | ||
| blackLivesToShow: PropTypes.number, | ||
| redLivesToShow: PropTypes.number, | ||
| lives: PropTypes.number, | ||
| }), | ||
| gameOverData: PropTypes.shape({ | ||
| userWon: PropTypes.bool, | ||
| }), | ||
| loading: PropTypes.bool, | ||
| storedData: PropTypes.array, | ||
| resetStoredData: PropTypes.func, | ||
| steps: PropTypes.number, | ||
| currentStep: PropTypes.number, | ||
| background: PropTypes.string, | ||
| children: PropTypes.node, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
PropTypes definitions need improvement.
While the addition of PropTypes is good for type safety, some improvements can be made:
- Complex object shapes (progressData, livesData, gameOverData) should have their nested properties fully defined
- Some props are missing validation for required props
Consider updating the PropTypes as follows:
MainLayout.propTypes = {
- level: PropTypes.number,
+ level: PropTypes.number.isRequired,
- handleNext: PropTypes.func,
+ handleNext: PropTypes.func.isRequired,
enableNext: PropTypes.bool,
showNext: PropTypes.bool,
showTimer: PropTypes.bool,
nextLessonAndHome: PropTypes.bool,
cardBackground: PropTypes.string,
backgroundImage: PropTypes.string,
points: PropTypes.number,
progressData: PropTypes.shape({
currentPracticeStep: PropTypes.number,
currentPracticeProgress: PropTypes.number,
+ // Add any other properties that progressData should have
}),
showProgress: PropTypes.bool,
- setOpenLangModal: PropTypes.func,
+ setOpenLangModal: PropTypes.func.isRequired,
- setOpenTestModal: PropTypes.func,
+ setOpenTestModal: PropTypes.func.isRequired,
lang: PropTypes.string,
handleBack: PropTypes.func,
disableScreen: PropTypes.bool,
isShowCase: PropTypes.bool,
startShowCase: PropTypes.bool,
contentType: PropTypes.string,
percentage: PropTypes.number,
fluency: PropTypes.bool,
setStartShowCase: PropTypes.func,
livesData: PropTypes.shape({
blackLivesToShow: PropTypes.number,
redLivesToShow: PropTypes.number,
lives: PropTypes.number,
+ // Add any other properties that livesData should have
}),
gameOverData: PropTypes.shape({
userWon: PropTypes.bool,
+ link: PropTypes.string, // Add this as it's used in the code
+ // Add any other properties that gameOverData should have
}),
loading: PropTypes.bool,
- storedData: PropTypes.array,
+ storedData: PropTypes.arrayOf(PropTypes.shape({
+ audioUrl: PropTypes.string,
+ correctAnswer: PropTypes.bool,
+ selectedAnswer: PropTypes.string
+ })),
resetStoredData: PropTypes.func,
steps: PropTypes.number,
currentStep: PropTypes.number,
background: PropTypes.string,
children: PropTypes.node,
pageName: PropTypes.string,
};Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style