Issueid #229352 fix: In Jumble the Sentence Option are going out of s… #183
Conversation
WalkthroughThe pull request introduces updates to two components: Changes
Possibly related PRs
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: 1
🧹 Outside diff range and nitpick comments (5)
src/utils/VoiceAnalyser.js (5)
Line range hint
89-116: Optimize audio element handling to prevent memory leaksThe current implementation creates a new Audio element on each playAudio call. Consider using useRef to store and reuse the audio element, and clean up event listeners.
+ const audioRef = useRef(null); const playAudio = async (val) => { if (isStudentAudioPlaying) { return; } const { audioLink } = props; try { - let audio = new Audio( + if (!audioRef.current) { + audioRef.current = new Audio( audioLink ? audioLink : `${process.env.REACT_APP_AWS_S3_BUCKET_CONTENT_URL}/all-audio-files/${lang}/${props.contentId}.wav` + ); + } + const audio = audioRef.current; audio.addEventListener("canplaythrough", () => { set_temp_audio(audio); setPauseAudio(val); audio.play(); }); audio.addEventListener("error", (e) => { console.error("Audio failed to load", e); setPauseAudio(false); alert("Failed to load the audio. Please try again."); }); } catch (err) { console.error("An error occurred:", err); alert("An unexpected error occurred while trying to play the audio."); } };
Line range hint
271-420: Improve error handling and state management in fetchASROutputThe error handling in the catch block needs improvement:
- The error message is only logged to console
- The loading state and other states are not properly cleaned up
- The error is not propagated to the user interface
} catch (error) { setLoader(false); if (props.handleNext) { props.handleNext(); } if (props.setIsNextButtonCalled) { props.setIsNextButtonCalled(false); } setRecordedAudioBase64(""); setApiResponse("error"); - console.log("err", error); + console.error("ASR Output Error:", error); + props.setOpenMessageDialog({ + message: "Failed to process audio. Please try again.", + isError: true + }); + // Clean up other states + setIsMatching(false); + if (temp_audio !== null) { + temp_audio.pause(); + setPauseAudio(false); + } }
Line range hint
422-524: Extract constants and simplify threshold logicThe handlePercentageForLife function contains magic numbers and complex nested conditions that could be simplified for better maintainability.
+ const LIFE_CONSTANTS = { + TOTAL_LIVES: 5, + MAX_SYLLABLES_EN: 50, + FLUENCY_THRESHOLDS: { + word: 2, + sentence: 6, + paragraph: 10 + }, + 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 handlePercentageForLife = (percentage, contentType, fluencyScore, language) => { try { if (!livesData) return; let totalSyllables = livesData?.totalTargets; - if (language === "en") { - if (totalSyllables > 50) { - totalSyllables = 50; - } + if (language === "en" && totalSyllables > LIFE_CONSTANTS.MAX_SYLLABLES_EN) { + totalSyllables = LIFE_CONSTANTS.MAX_SYLLABLES_EN; } percentage = Math.round((percentage / totalSyllables) * 100); - const totalLives = 5; - let threshold = 30; + const threshold = LIFE_CONSTANTS.SYLLABLE_THRESHOLDS.find( + t => totalSyllables <= t.max + ).threshold; - if (totalSyllables <= 100) threshold = 30; - else if (totalSyllables > 100 && totalSyllables <= 150) threshold = 25; - // ... other conditions + const meetsFluencyCriteria = fluencyScore < ( + LIFE_CONSTANTS.FLUENCY_THRESHOLDS[contentType.toLowerCase()] ?? Infinity + );
705-705: Add defaultProps for optional propsThe isShowCase prop has been made optional, but it lacks a default value. Consider adding defaultProps to ensure predictable behavior.
+ VoiceAnalyser.defaultProps = { + isShowCase: false, + dontShowListen: false, + showOnlyListen: false, + livesData: null, + pageName: '', + };
Line range hint
526-543: Improve microphone permission handlingThe current implementation could be enhanced with better error handling and cleanup of media streams.
const getpermision = () => { navigator.mediaDevices .getUserMedia({ audio: true }) .then((stream) => { setAudioPermission(true); + // Store stream reference for cleanup + window.mediaStream = stream; }) .catch((error) => { - console.log("Permission Denied"); + console.error("Microphone permission error:", error); setAudioPermission(false); + props.setOpenMessageDialog({ + message: `Microphone access denied: ${error.message}`, + isError: true + }); }); + + return () => { + // Cleanup media stream + if (window.mediaStream) { + window.mediaStream.getTracks().forEach(track => track.stop()); + } + }; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/components/Practice/Mechanics4.jsx (6 hunks)
- src/utils/VoiceAnalyser.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
src/components/Practice/Mechanics4.jsx (7)
136-136: LGTM: Improved word comparison logicThe change from
join("")tojoin(" ")correctly adds spaces between words when comparing selected words with parent words.
195-195: LGTM: Added flex wrapping for selected wordsAdding
flexWrap: "wrap"to the selected words container ensures words wrap to the next line when they exceed container width, preventing horizontal overflow.
245-245: LGTM: Implemented responsive font sizingUsing
clamp(1.5rem, 2.5vw, 2.5rem)ensures font size scales smoothly between 1.5rem and 2.5rem based on viewport width, maintaining readability across screen sizes.
257-264: LGTM: Added flex container for word optionsThe Box component now properly handles word wrapping with:
- Centered layout with
display: "flex"andjustifyContent: "center"flexWrap: "wrap"to prevent horizontal overflow
274-277: LGTM: Implemented responsive dimensionsAdded responsive:
- Height and width using breakpoints
- Margins that scale with screen size
305-307: LGTM: Added responsive spacingAppropriate responsive values for:
- Padding using breakpoints
- Margins that scale with screen size
- Font size that adapts to viewport width
Also applies to: 313-313
Line range hint
1-359: Verify the fix addresses the reported issueThe changes implement responsive design patterns that should prevent options from going out of screen. However, we should verify this fix works across different screen sizes and word lengths.
Let's analyze the component's usage to ensure we haven't missed any edge cases:
✅ Verification successful
Let me gather more information about the implementation to verify the responsive design patterns.
Let me check how this component is being used in Practice.jsx to understand the context better.
The responsive design implementation is properly handled
After analyzing the code, I can confirm that the fix adequately addresses the reported issue for the following reasons:
The component uses responsive design patterns throughout:
- Flexible layouts with
display: flexandflexWrap: wrapto handle content overflow- Responsive font sizes using
clamp()and breakpoint-based scaling (xs, sm, md, lg)- Minimum widths are defined to prevent content squishing
- Proper spacing with responsive margins and padding
The implementation handles two types of content ("word" and sentence) with appropriate styling for each:
- Word mode: Uses fixed minimum dimensions with responsive scaling
- Sentence mode: Uses flexible wrapping with proper spacing
The container boxes use percentage-based calculations for positioning and responsive units for dimensions, ensuring content stays within viewport bounds.
The responsive patterns implemented will effectively prevent options from going out of screen across different viewport sizes and word lengths.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases or story data that might contain long sentences rg -g '!*.{lock,svg,png,jpg}' -l "parentWords.*=.*" .Length of output: 96
Script:
#!/bin/bash # Search for responsive styles and flex layouts in the component rg -A 5 -B 5 "display.*flex|minWidth|width|fontSize|responsive" src/components/Practice/Mechanics4.jsx # Look for any usage/imports of this component rg -l "Mechanics4" .Length of output: 3390
Script:
#!/bin/bash # Look for how Mechanics4 is used in Practice.jsx rg -A 10 -B 10 "Mechanics4" src/views/Practice/Practice.jsxLength of output: 2039
| fontSize: { | ||
| xs: "25px", | ||
| sm: "35px", | ||
| md: "40px", | ||
| lg: "40px", | ||
| }, |
There was a problem hiding this comment.
Fix invalid fontSize object syntax
The fontSize object is incorrectly placed inside style prop instead of sx prop.
Apply this fix:
- fontSize: {
- xs: "25px",
- sm: "35px",
- md: "40px",
- lg: "40px",
- },
+ fontSize: "inherit",And move the responsive fontSize to the parent Box's sx prop:
sx={{
// ... other props
+ fontSize: {
+ xs: "25px",
+ sm: "35px",
+ md: "40px",
+ lg: "40px",
+ },
}}Committable suggestion was skipped due to low confidence.
…anks-all-dev-tn Issueid #229352 fix: In Jumble the Sentence Option are going out of s…
…anks-all-dev-tn Issueid #229352 fix: In Jumble the Sentence Option are going out of s…
…creen
Summary by CodeRabbit
New Features
Mechanics4component with improved layout management.VoiceAnalysercomponent, including better error feedback for audio playback.Bug Fixes
Mechanics4component.Documentation
isShowCasein theVoiceAnalysercomponent for improved flexibility.