fix: harden state handling and stabilize gradient rendering in Electron#622
fix: harden state handling and stabilize gradient rendering in Electron#622ashi2004 wants to merge 3 commits intoAOSSIE-Org:mainfrom
Conversation
📝 WalkthroughWalkthroughThese changes address gradient rendering inconsistencies and component stability. The desktop app disables GPU hardware acceleration to resolve rendering issues. The web UI refactors gradient styling into dedicated CSS classes and converts the Output component's dropdown management from direct DOM manipulation to React state-based control. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 (2)
eduaid_web/src/pages/Output.jsx (2)
93-96:⚠️ Potential issue | 🟡 MinorlocalStorage parsing can still throw on malformed data.
The PR objective mentions "preventing crashes from unsafe localStorage parsing," but
JSON.parse()will still throw aSyntaxErrorif localStorage contains malformed JSON. Consider wrapping in try-catch.🛡️ Proposed fix for safer parsing
useEffect(() => { - const qaPairsFromStorage = - JSON.parse(localStorage.getItem("qaPairs")) || {}; - if (qaPairsFromStorage) { + let qaPairsFromStorage = {}; + try { + qaPairsFromStorage = JSON.parse(localStorage.getItem("qaPairs")) || {}; + } catch (e) { + console.error("Failed to parse qaPairs from localStorage:", e); + } + if (Object.keys(qaPairsFromStorage).length > 0) { const combinedQaPairs = [];🤖 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 93 - 96, The JSON.parse call inside the useEffect that reads localStorage.getItem("qaPairs") can throw on malformed data; update the useEffect to wrap parsing in a try-catch around JSON.parse(localStorage.getItem("qaPairs")) (used to set qaPairsFromStorage), fall back to an empty object on error, and optionally handle/log the error or remove the invalid "qaPairs" entry so the app doesn't crash when malformed data is present.
123-133:⚠️ Potential issue | 🟠 MajorPotential TypeError if
qaPairsFromStorage["output"]is undefined.The condition on line 123 checks for
output_mcqorquestionType === "get_mcq", but line 124 accessesqaPairsFromStorage["output"]without verifying it exists. This can throw if theoutputkey is missing.🐛 Proposed fix to guard against undefined
- if (qaPairsFromStorage["output_mcq"] || questionType === "get_mcq") { + if ((qaPairsFromStorage["output_mcq"] || questionType === "get_mcq") && qaPairsFromStorage["output"]) { qaPairsFromStorage["output"].forEach((qaPair) => {🤖 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 - 133, The current block that handles MCQ merging reads qaPairsFromStorage["output"] without ensuring it exists, which can throw a TypeError; update the conditional around qaPairsFromStorage["output_mcq"] / questionType === "get_mcq" to also guard that qaPairsFromStorage["output"] is an array (e.g., check Array.isArray(qaPairsFromStorage["output"]) or qaPairsFromStorage["output"] != null) before calling forEach, and only call combinedQaPairs.push(...) inside that guarded branch so functions/values like qaPairsFromStorage, combinedQaPairs, and the forEach callback on qaPair are safe.
🧹 Nitpick comments (3)
eduaid_desktop/main.js (1)
2-3: Make hardware acceleration disable configurable instead of unconditional.This disables GPU globally for all users and environments. While it may stabilize specific rendering glitches, Electron's official documentation recommends using
disableHardwareAcceleration()only when experiencing GPU-related issues—not by default for normal windows, as it can degrade performance and visual quality (especially on a gradient-heavy UI like this). Use the existing config system to gate it behind a flag (environment or config option) so it applies only where needed.Suggested refactor
// Fix Electron rendering glitch (safe and stable) -app.disableHardwareAcceleration(); +if (config.disableGpuAcceleration === true) { + app.disableHardwareAcceleration(); +}Then add
disableGpuAcceleration: falseto the appropriate environment block inconfig.js.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eduaid_desktop/main.js` around lines 2 - 3, Replace the unconditional call to app.disableHardwareAcceleration() with a configuration-gated call: add a boolean config flag (e.g., disableGpuAcceleration) to your existing config system (config.js) and read that flag at startup; only call app.disableHardwareAcceleration() when that flag is true. Locate the startup boot code where app.disableHardwareAcceleration() is invoked and wrap it with a conditional check against your config (e.g., config.disableGpuAcceleration) so the behavior is environment/config driven rather than unconditional.eduaid_web/src/pages/Output.jsx (2)
202-205: Dropdown not closed on worker error.The success path (line 198) closes the dropdown via
setShowDropdown(false), but the error handler doesn't. Consider closing the dropdown on error for consistent UX.♻️ Proposed fix
worker.onerror = (err) => { console.error("PDF generation failed in worker:", err); + setShowDropdown(false); worker.terminate(); };🤖 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 202 - 205, The error handler for PDF worker (worker.onerror) currently logs and terminates the worker but doesn't close the UI dropdown; update the worker.onerror callback to call setShowDropdown(false) before or after worker.terminate() so the dropdown is closed on errors just like the success path (see the success handler usage of setShowDropdown(false)), keeping the existing console.error and worker.terminate() behavior.
382-392: Clean state-driven dropdown implementation.Good use of
reffor outside-click detection and functional state update for toggle. Note: theid="pdfDropdown"attribute (line 391) appears to be a leftover from the previous DOM-based approach and can be removed.🧹 Optional cleanup
<div - id="pdfDropdown" className={`${showDropdown ? 'block' : 'hidden'} absolute bottom-full mb-1 left-0 sm:left-auto right-0 sm:right-auto bg-[`#02000F`] shadow-md text-white rounded-lg shadow-lg z-50 w-full sm:w-48`} >🤖 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 382 - 392, The id="pdfDropdown" is a leftover DOM-based hook and should be removed to avoid confusion; locate the dropdown container element that uses dropdownRef and controlled by showDropdown (the div with className using `${showDropdown ? 'block' : 'hidden'}`) and delete the id="pdfDropdown" attribute, keeping the ref={dropdownRef} and the state toggle via setShowDropdown intact.
🤖 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/pages/Home.jsx`:
- Around line 50-51: Replace the two separate <h1> elements in the Home
component with a single <h1> containing two <span> children (one for "Edu" and
one for "Aid") to preserve visual styles while fixing semantic accessibility;
keep the existing class names (gradient-text-base plus gradient-text for the
first span and gradient-text-base plus gradient-text-2 for the second) and
mirror the pattern used in Output.jsx (the single <h1> with spans) so screen
readers and SEO see one primary heading.
---
Outside diff comments:
In `@eduaid_web/src/pages/Output.jsx`:
- Around line 93-96: The JSON.parse call inside the useEffect that reads
localStorage.getItem("qaPairs") can throw on malformed data; update the
useEffect to wrap parsing in a try-catch around
JSON.parse(localStorage.getItem("qaPairs")) (used to set qaPairsFromStorage),
fall back to an empty object on error, and optionally handle/log the error or
remove the invalid "qaPairs" entry so the app doesn't crash when malformed data
is present.
- Around line 123-133: The current block that handles MCQ merging reads
qaPairsFromStorage["output"] without ensuring it exists, which can throw a
TypeError; update the conditional around qaPairsFromStorage["output_mcq"] /
questionType === "get_mcq" to also guard that qaPairsFromStorage["output"] is an
array (e.g., check Array.isArray(qaPairsFromStorage["output"]) or
qaPairsFromStorage["output"] != null) before calling forEach, and only call
combinedQaPairs.push(...) inside that guarded branch so functions/values like
qaPairsFromStorage, combinedQaPairs, and the forEach callback on qaPair are
safe.
---
Nitpick comments:
In `@eduaid_desktop/main.js`:
- Around line 2-3: Replace the unconditional call to
app.disableHardwareAcceleration() with a configuration-gated call: add a boolean
config flag (e.g., disableGpuAcceleration) to your existing config system
(config.js) and read that flag at startup; only call
app.disableHardwareAcceleration() when that flag is true. Locate the startup
boot code where app.disableHardwareAcceleration() is invoked and wrap it with a
conditional check against your config (e.g., config.disableGpuAcceleration) so
the behavior is environment/config driven rather than unconditional.
In `@eduaid_web/src/pages/Output.jsx`:
- Around line 202-205: The error handler for PDF worker (worker.onerror)
currently logs and terminates the worker but doesn't close the UI dropdown;
update the worker.onerror callback to call setShowDropdown(false) before or
after worker.terminate() so the dropdown is closed on errors just like the
success path (see the success handler usage of setShowDropdown(false)), keeping
the existing console.error and worker.terminate() behavior.
- Around line 382-392: The id="pdfDropdown" is a leftover DOM-based hook and
should be removed to avoid confusion; locate the dropdown container element that
uses dropdownRef and controlled by showDropdown (the div with className using
`${showDropdown ? 'block' : 'hidden'}`) and delete the id="pdfDropdown"
attribute, keeping the ref={dropdownRef} and the state toggle via
setShowDropdown intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3154c01-0cc1-4ff1-a675-94df61a08973
📒 Files selected for processing (4)
eduaid_desktop/main.jseduaid_web/src/index.csseduaid_web/src/pages/Home.jsxeduaid_web/src/pages/Output.jsx
| <h1 className="gradient-text-base gradient-text">Edu</h1> | ||
| <h1 className="gradient-text-base gradient-text-2">Aid</h1> |
There was a problem hiding this comment.
Semantic issue: Two <h1> elements for a single heading.
Using two separate <h1> elements for "Edu" and "Aid" is semantically incorrect—screen readers and search engines will interpret these as two distinct primary headings. This differs from the approach in Output.jsx (lines 220-223) which correctly uses a single <h1> with two <span> children.
🛠️ Proposed fix to use a single h1 with spans
- <h1 className="gradient-text-base gradient-text">Edu</h1>
- <h1 className="gradient-text-base gradient-text-2">Aid</h1>
+ <h1 className="text-4xl sm:text-6xl font-extrabold flex gap-1 justify-center">
+ <span className="gradient-text-base gradient-text">Edu</span>
+ <span className="gradient-text-base gradient-text-2">Aid</span>
+ </h1>📝 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.
| <h1 className="gradient-text-base gradient-text">Edu</h1> | |
| <h1 className="gradient-text-base gradient-text-2">Aid</h1> | |
| <h1 className="text-4xl sm:text-6xl font-extrabold flex gap-1 justify-center"> | |
| <span className="gradient-text-base gradient-text">Edu</span> | |
| <span className="gradient-text-base gradient-text-2">Aid</span> | |
| </h1> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@eduaid_web/src/pages/Home.jsx` around lines 50 - 51, Replace the two separate
<h1> elements in the Home component with a single <h1> containing two <span>
children (one for "Edu" and one for "Aid") to preserve visual styles while
fixing semantic accessibility; keep the existing class names (gradient-text-base
plus gradient-text for the first span and gradient-text-base plus
gradient-text-2 for the second) and mirror the pattern used in Output.jsx (the
single <h1> with spans) so screen readers and SEO see one primary heading.
1 participant
Addressed Issues:
Fixes #621
This PR tightens Output page behavior and fixes gradient text rendering inconsistencies seen in Electron.
What I changed
Why
Additional Notes:
AI Usage Disclosure:
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
Checklist
Summary by CodeRabbit
Release Notes
Style
Chores