fix:621 fixed gradient issues#653
Open
MANVENDRA-github wants to merge 2 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDisabled Electron hardware acceleration, added CSS gradient text utilities, and refactored the Output PDF dropdown to use React state and a ref-based outside-click handler instead of imperative DOM manipulation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
eduaid_web/src/pages/Output.jsx (3)
94-96:⚠️ Potential issue | 🟠 MajorProtect localStorage parse on Line 95 to avoid runtime crashes.
JSON.parse(localStorage.getItem("qaPairs"))will throw on malformed data, which breaks initial render instead of degrading gracefully.Proposed fix
- const qaPairsFromStorage = - JSON.parse(localStorage.getItem("qaPairs")) || {}; + let qaPairsFromStorage = {}; + try { + const raw = localStorage.getItem("qaPairs"); + qaPairsFromStorage = raw ? JSON.parse(raw) : {}; + } catch (error) { + console.error("Invalid qaPairs in localStorage:", error); + qaPairsFromStorage = {}; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eduaid_web/src/pages/Output.jsx` around lines 94 - 96, Wrap the JSON.parse(localStorage.getItem("qaPairs")) call in a safe parse so a malformed value doesn't throw during initial render: catch errors from parsing the result of localStorage.getItem("qaPairs") (or check it's valid JSON) and fallback to an empty object; update the qaPairsFromStorage initialization in Output.jsx (the const qaPairsFromStorage and the subsequent if (qaPairsFromStorage) check) to use the safe result so the component degrades gracefully instead of crashing.
123-132:⚠️ Potential issue | 🟠 MajorGuard
outputbefore iterating and dedupe merged MCQ datasets.Line 124 assumes
qaPairsFromStorage["output"]is always an array; when absent/corrupt it will throw. Also, appending both MCQ sources without dedupe can render duplicates.Proposed fix
- if (qaPairsFromStorage["output_mcq"] || questionType === "get_mcq") { - qaPairsFromStorage["output"].forEach((qaPair) => { - combinedQaPairs.push({ - question: qaPair.question_statement, - question_type: "MCQ", - options: qaPair.options, - answer: qaPair.answer, - context: qaPair.context, - }); - }); - } + const mcqSeen = new Set(); + const pushMcq = (qaPair) => { + const mapped = { + question: qaPair.question_statement, + question_type: "MCQ", + options: qaPair.options, + answer: qaPair.answer, + context: qaPair.context, + }; + const key = `${mapped.question}::${mapped.answer}`; + if (!mcqSeen.has(key)) { + mcqSeen.add(key); + combinedQaPairs.push(mapped); + } + }; + + if (qaPairsFromStorage["output_mcq"]?.questions?.length) { + qaPairsFromStorage["output_mcq"].questions.forEach(pushMcq); + } + if (questionType === "get_mcq" && Array.isArray(qaPairsFromStorage["output"])) { + qaPairsFromStorage["output"].forEach(pushMcq); + }Based on learnings: In
eduaid_web/src/pages/Output.jsx,output_mcqandoutput(get_mcq) should be merged as separate MCQ sources without duplicates or order inconsistencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eduaid_web/src/pages/Output.jsx` around lines 123 - 132, Guard access to qaPairsFromStorage["output"] and dedupe when merging MCQ sources: before iterating in Output.jsx ensure qaPairsFromStorage["output"] is an array (e.g., default to [] when falsy or not Array.isArray) and only push MCQ entries to combinedQaPairs after normalizing them (use question_statement as the primary unique key, optionally combined with serialized options) to filter out duplicates when merging from qaPairsFromStorage["output_mcq"] and qaPairsFromStorage["output"] (or when questionType === "get_mcq"); keep insertion order by checking a Set of seen keys before pushing to combinedQaPairs.
183-206:⚠️ Potential issue | 🟠 MajorPrevent overlapping PDF workers and add unmount-safe cleanup.
Current flow can spawn concurrent workers if users click export options repeatedly; worker lifetime is not tracked across unmount/navigation, which can leak work and trigger stale updates.
Proposed fix
+ const pdfWorkerRef = useRef(null); + const [isGeneratingPdf, setIsGeneratingPdf] = useState(false); + + useEffect(() => { + return () => { + if (pdfWorkerRef.current) { + pdfWorkerRef.current.terminate(); + pdfWorkerRef.current = null; + } + }; + }, []); + - const generatePDF = async (mode) => { + const generatePDF = async (mode) => { + if (isGeneratingPdf) return; + setIsGeneratingPdf(true); + setShowDropdown(false); const logoBytes = await loadLogoAsBytes(); + if (pdfWorkerRef.current) { + pdfWorkerRef.current.terminate(); + } const worker = new Worker(new URL("../workers/pdfWorker.js", import.meta.url), { type: "module" }); + pdfWorkerRef.current = worker; @@ - setShowDropdown(false); worker.terminate(); + pdfWorkerRef.current = null; + setIsGeneratingPdf(false); }; @@ worker.onerror = (err) => { console.error("PDF generation failed in worker:", err); + setShowDropdown(false); worker.terminate(); + pdfWorkerRef.current = null; + setIsGeneratingPdf(false); }; };Also applies to: 389-413
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eduaid_web/src/pages/Output.jsx` around lines 183 - 206, The generatePDF function can spawn overlapping Web Workers and leaks them across unmounts; fix by tracking the active worker in a ref (e.g., currentWorkerRef) instead of a local variable, guard generatePDF to return early if currentWorkerRef.current exists, attach onmessage/onerror that clear currentWorkerRef.current and only call setShowDropdown if the component is still mounted, and add a useEffect cleanup to terminate any currentWorkerRef.current and remove handlers on unmount; update references to worker in generatePDF and any other export handlers (also where similar code appears around the other block) to use this ref-based lifecycle.
🧹 Nitpick comments (1)
eduaid_desktop/main.js (1)
2-2: Move hardware-acceleration disablement to configuration, not unconditional startup.Line 2 applies a global workaround that degrades performance on machines with stable rendering. The codebase already has a
config.jsmodule handling environment-specific settings—extend it to make this conditional (e.g.,config.disableHardwareAccelerationflag toggled per environment or via environment variable). This avoids unnecessary performance loss on systems where GPU acceleration works reliably.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eduaid_desktop/main.js` at line 2, The unconditional call to app.disableHardwareAcceleration() should be made conditional via the existing configuration: add a boolean flag (e.g., disableHardwareAcceleration) to the config.js module and read it where the Electron app is initialized; replace the direct call to app.disableHardwareAcceleration() with a guarded call that only executes when config.disableHardwareAcceleration is true (or when the corresponding env var enables it). Ensure config.js exports the new flag (derived from env or environment-specific settings) and update the startup code that currently calls app.disableHardwareAcceleration() to use that flag before invoking the method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eduaid_web/src/index.css`:
- Around line 27-33: The .gradient-text-base rule is missing the standard color
property needed for Firefox compatibility; update the CSS for the
.gradient-text-base class to add color: transparent alongside
-webkit-text-fill-color: transparent so background-clip: text renders the
gradient correctly across browsers.
---
Outside diff comments:
In `@eduaid_web/src/pages/Output.jsx`:
- Around line 94-96: Wrap the JSON.parse(localStorage.getItem("qaPairs")) call
in a safe parse so a malformed value doesn't throw during initial render: catch
errors from parsing the result of localStorage.getItem("qaPairs") (or check it's
valid JSON) and fallback to an empty object; update the qaPairsFromStorage
initialization in Output.jsx (the const qaPairsFromStorage and the subsequent if
(qaPairsFromStorage) check) to use the safe result so the component degrades
gracefully instead of crashing.
- Around line 123-132: Guard access to qaPairsFromStorage["output"] and dedupe
when merging MCQ sources: before iterating in Output.jsx ensure
qaPairsFromStorage["output"] is an array (e.g., default to [] when falsy or not
Array.isArray) and only push MCQ entries to combinedQaPairs after normalizing
them (use question_statement as the primary unique key, optionally combined with
serialized options) to filter out duplicates when merging from
qaPairsFromStorage["output_mcq"] and qaPairsFromStorage["output"] (or when
questionType === "get_mcq"); keep insertion order by checking a Set of seen keys
before pushing to combinedQaPairs.
- Around line 183-206: The generatePDF function can spawn overlapping Web
Workers and leaks them across unmounts; fix by tracking the active worker in a
ref (e.g., currentWorkerRef) instead of a local variable, guard generatePDF to
return early if currentWorkerRef.current exists, attach onmessage/onerror that
clear currentWorkerRef.current and only call setShowDropdown if the component is
still mounted, and add a useEffect cleanup to terminate any
currentWorkerRef.current and remove handlers on unmount; update references to
worker in generatePDF and any other export handlers (also where similar code
appears around the other block) to use this ref-based lifecycle.
---
Nitpick comments:
In `@eduaid_desktop/main.js`:
- Line 2: The unconditional call to app.disableHardwareAcceleration() should be
made conditional via the existing configuration: add a boolean flag (e.g.,
disableHardwareAcceleration) to the config.js module and read it where the
Electron app is initialized; replace the direct call to
app.disableHardwareAcceleration() with a guarded call that only executes when
config.disableHardwareAcceleration is true (or when the corresponding env var
enables it). Ensure config.js exports the new flag (derived from env or
environment-specific settings) and update the startup code that currently calls
app.disableHardwareAcceleration() to use that flag before invoking the method.
🪄 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: 87949d97-a157-4c07-8daa-af5be63481f7
📒 Files selected for processing (3)
eduaid_desktop/main.jseduaid_web/src/index.csseduaid_web/src/pages/Output.jsx
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.
This PR addresses rendering inconsistencies in the Electron application and improves the overall stability of the Output component.
The changes focus on fixing gradient text rendering issues and resolving multiple reliability concerns related to state handling, localStorage parsing, dropdown interactions, and Web Worker lifecycle management.
Related Issue
Closes: #621
Type of Change
Bug fix
Summary by CodeRabbit
New Features
Improvements