chore: enforce code formatting with Prettier#440
Open
aryash45 wants to merge 1 commit intoAOSSIE-Org:mainfrom
Open
chore: enforce code formatting with Prettier#440aryash45 wants to merge 1 commit intoAOSSIE-Org:mainfrom
aryash45 wants to merge 1 commit intoAOSSIE-Org:mainfrom
Conversation
Author
|
@varunchitre15 @Zahnentferner please review this pr |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEstablishes project-wide code formatting consistency by adding Prettier configuration files (.prettierrc with formatting rules, .prettierignore with exclusions), npm scripts for formatting and validation across three subprojects (extension, eduaid_web, eduaid_desktop), and a GitHub Actions workflow to enforce formatting checks on pull requests. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 8
🧹 Nitpick comments (9)
.gitignore (1)
24-31:extension/node_modules(line 24) is now redundantThe new
node_modules/pattern (line 31) matches anynode_modulesdirectory at any depth — includingextension/node_modules/— making the path-specific entry on line 24 a no-op.🧹 Optional cleanup
-extension/node_modules backend/credentials.json backend/token.json backend/service_account_key.json venv backend/Eduaid .DS_Store node_modules/No
node_modulesdirectories are currently tracked in git, so adding thenode_modules/pattern is safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 24 - 31, The .gitignore contains a redundant entry "extension/node_modules" because the broader "node_modules/" pattern already ignores any node_modules at any depth; remove the specific "extension/node_modules" line so only the general "node_modules/" entry remains, leaving other entries like "backend/credentials.json" and "venv" unchanged.package.json (1)
10-10: Align Prettier version across the workspace.The root pins
^3.2.5whileextension/package.jsonandeduaid_web/package.jsonpin^3.8.1. With lockfiles in CI, different resolved versions can produce divergent formatting, makingformat:checkflaky across sub-packages.♻️ Proposed fix — pin consistently at the newest minimum
- "prettier": "^3.2.5" + "prettier": "^3.8.1"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 10, Update the Prettier version in the root package.json to match the workspace packages: change the "prettier" dependency (currently "^3.2.5") to the newer pinned version used by extension/package.json and eduaid_web/package.json ("^3.8.1") so all subpackages resolve the same Prettier release and avoid formatting divergences in CI..prettierrc (1)
1-11: Consider adding"endOfLine": "lf"for cross-platform consistency.Without an explicit
endOfLine, the behavior differs between Prettier v2 ("auto", preserves existing line endings) and v3 ("lf"). On Windows developer machines with Prettier v2, files checked out with CRLF endings will be left with CRLF, which can cause spurious diffs in CI.✨ Proposed addition
{ "semi": true, "singleQuote": false, "jsxSingleQuote": false, "tabWidth": 2, "trailingComma": "es5", "printWidth": 80, "bracketSpacing": true, "bracketSameLine": false, - "arrowParens": "always" + "arrowParens": "always", + "endOfLine": "lf" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.prettierrc around lines 1 - 11, Add an explicit endOfLine setting to the Prettier config: update the .prettierrc JSON to include the "endOfLine" property set to "lf" so that Prettier behavior is consistent across platforms and versions; modify the existing config object (the entry with keys like "semi", "singleQuote", "tabWidth", etc.) to add "endOfLine": "lf"..prettierignore (1)
4-4: Consider also ignoring*.min.css.
*.min.jsis excluded but minified CSS files are not. For symmetry and to avoid accidentally reformatting vendored/generated minified stylesheets:✨ Proposed addition
*.min.js +*.min.css🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.prettierignore at line 4, Add the minified CSS ignore pattern to .prettierignore so vendored/generated *.min.css files are skipped like *.min.js; update the .prettierignore file by adding the pattern "*.min.css" alongside the existing "*.min.js" entry to prevent Prettier from reformatting minified stylesheets.extension/src/pages/previous/Previous.jsx (1)
2-2: MigrateReactDOM.rendertocreateRootfor React 18+ compatibility.Line 108 uses the legacy
ReactDOM.renderAPI (imported on line 2), which is deprecated in React 18 and removed in React 19. The project uses React 18.2.0, so this migration is recommended before any future upgrade.♻️ Proposed migration
import React from "react"; -import ReactDOM from "react-dom"; +import { createRoot } from "react-dom/client";-ReactDOM.render(<Previous />, document.getElementById("root")); +const container = document.getElementById("root"); +const root = createRoot(container); +root.render(<Previous />);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/src/pages/previous/Previous.jsx` at line 2, Replace the legacy ReactDOM.render usage: change the import from "react-dom" to use createRoot from "react-dom/client" (replace ReactDOM import), locate the ReactDOM.render call (ReactDOM.render(...)) and replace it by creating a root with createRoot(container) and calling .render(...) on that root (keep the same root container element and the same component tree), ensuring ReactDOM references are removed and the new createRoot API is used in Previous.jsx.eduaid_desktop/package.json (1)
33-33: Align Prettier version witheduaid_web(^3.8.1).
eduaid_desktoppins"^3.2.5"whileeduaid_webuses"^3.8.1". The latest published Prettier version is 3.8.1. Both ranges currently resolve to the same installed version, but declaring different minimums is semantically inconsistent and creates divergence risk if a lockfile pins the older minimum in a fresh environment.🔧 Proposed fix
- "prettier": "^3.2.5", + "prettier": "^3.8.1",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eduaid_desktop/package.json` at line 33, The package.json in eduaid_desktop declares "prettier": "^3.2.5" which diverges from eduaid_web's "^3.8.1"; update the Prettier dependency string in eduaid_desktop's package.json (the "prettier" entry) to "^3.8.1" so both projects declare the same minimum compatible version, then run install/update lockfile (e.g., npm install or yarn install) to refresh the lockfile.LICENSE.md (1)
1-1: Consider addingLICENSE.mdto.prettierignore.Prettier has altered this GPL v3 document's raw formatting (added a
#Markdown header, converted*bullets to-). The license preamble explicitly states it must be reproduced verbatim; reformatting the source file, even cosmetically, risks violating that requirement.🛡️ Proposed fix
Add to
.prettierignore:+LICENSE.md🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LICENSE.md` at line 1, Prettier is reformatting the GPLv3 license file (LICENSE.md) which must be preserved verbatim; update the repository's formatting ignore list by adding "LICENSE.md" to .prettierignore so Prettier skips this file. Locate the .prettierignore file (or create it if missing) and add a line with LICENSE.md, commit the change so the linter/formatter will no longer modify the LICENSE.md file.extension/public/background.js (2)
17-57: "askExtension" handler logic is reasonable overall, but note the redundant storage write.Line 21-23 stores
selectedTextviachrome.storage.local.set, then line 32-34 sends the same text to the content script, which in turn (incontentScript.jsline 9) writesselectedTexttochrome.storage.localagain. This double-write is unnecessary — consider removing the storage write fromcontentScript.jsor from here to avoid redundancy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/public/background.js` around lines 17 - 57, The context menu handler (chrome.contextMenus.onClicked) and contentScript.js both write the same selectedText to chrome.storage.local, causing a redundant double-write; remove one of these writes—either delete the chrome.storage.local.set({ selectedText: ... }) call in the onClicked handler or remove the write in contentScript.js (line that writes selectedText) and rely on the remaining flow where the selection is sent via chrome.tabs.sendMessage to contentScript.js; ensure whichever write you keep still provides the selectedText to consumers (via storage or message) and update any dependent logic accordingly.
1-14: Wrap context menu creation inchrome.runtime.onInstalledto prevent duplicate ID errors on service worker restart.Context menus persist across extension reloads and service worker restarts. Calling
chrome.contextMenus.create()at the top level will fail with a duplicate ID error if the service worker code re-executes. Usechrome.runtime.onInstalled.addListener()to safely create menus only when the extension is installed or updated.Proposed fix
-// Create context menus -chrome.contextMenus.create({ - id: "askExtension", - title: "Generate Quiz with Selected Text", - contexts: ["selection"], -}); - -// YouTube-specific quiz generator -chrome.contextMenus.create({ - id: "generateQuizFromYouTube", - title: "Generate Quiz from YouTube Video", - contexts: ["page"], - documentUrlPatterns: ["*://www.youtube.com/watch?v=*"], -}); +// Create context menus on install/update +chrome.runtime.onInstalled.addListener(() => { + chrome.contextMenus.create({ + id: "askExtension", + title: "Generate Quiz with Selected Text", + contexts: ["selection"], + }); + + chrome.contextMenus.create({ + id: "generateQuizFromYouTube", + title: "Generate Quiz from YouTube Video", + contexts: ["page"], + documentUrlPatterns: ["*://www.youtube.com/watch?v=*"], + }); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/public/background.js` around lines 1 - 14, The context menu creation is executed at top-level and can cause duplicate ID errors on service worker restart; move the two chrome.contextMenus.create calls (the one creating id "askExtension" and the one creating id "generateQuizFromYouTube") into a chrome.runtime.onInstalled.addListener callback so menus are created only on install/update, i.e., wrap both create calls inside chrome.runtime.onInstalled.addListener(() => { ... }) and remove the top-level creates to prevent duplicate ID collisions on reloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/prettier.yml:
- Around line 20-23: The workflow uses actions/setup-node@v4 with node-version:
18 which is EOL; update the Node version to a supported LTS (set node-version:
20 or 22) in the same job step to restore security updates. Locate the step
using actions/setup-node@v4 and change the with: node-version value from 18 to
20 (or 22) and run CI to ensure no test/lockfile issues; optionally update the
setup-node action pin if you want a newer minor/major release.
- Around line 25-41: The three sequential steps named "Check extension
formatting", "Check eduaid_web formatting", and "Check eduaid_desktop
formatting" should be made robust and independent: replace the fragile "npm
install" with a reproducible install (prefer "npm ci" when a lockfile exists or
"npm install --legacy-peer-deps" as a fallback) in each step, and restructure
the job into a matrix (or separate jobs) so each project runs its own job (e.g.,
a matrix variable like matrix.project used with "cd ${{ matrix.project }} && npm
ci || npm install --legacy-peer-deps && npm run format:check") ensuring all
three checks execute independently and report results even if one fails.
In `@eduaid_web/package.json`:
- Around line 25-27: The Prettier dependency is declared with a caret range
causing inconsistent formatting across projects; update the package.json entries
that define the "prettier" dependency (the dependency/devDependency named
"prettier") to an exact version string "3.8.1" (remove the leading ^) and ensure
all subprojects use the same exact version, then keep the "format" and
"format:check" scripts as-is; specifically change any "prettier": "^3.8.1" or
"^3.2.5" occurrences to "prettier": "3.8.1" so all package.json manifests are
aligned.
In `@eduaid_web/src/index.css`:
- Around line 7-9: The font-family declaration contains unnecessary quotes
around single-word font names; update the font-family property (the font-family
rule in index.css) to remove quotes from "Roboto", "Oxygen", "Ubuntu", and
"Cantarell" while keeping quotes only for multi-word names like "Segoe UI",
"Fira Sans", "Droid Sans", and "Helvetica Neue" so it conforms to the
font-family-name-quotes Stylelint rule.
In `@extension/public/background.js`:
- Around line 60-91: The guard in the background.js handler for menuItemId
"generateQuizFromYouTube" relies on window.youtubeScriptLoaded but
youtubeContentScript.js never sets it; open youtubeContentScript.js and add a
top-level assignment to set window.youtubeScriptLoaded = true (so the
chrome.scripting.executeScript check in the background.js block using
executeScript(func: () => window.youtubeScriptLoaded || false) will return true
after first injection), ensuring the flag is set in the page context before any
other script logic runs.
- Around line 110-113: The chrome.action.onClicked listener is dead because
chrome.action.setPopup is registered globally (chrome.action.setPopup({ popup:
"src/popup/popup.html" })), so choose one fix: either remove the
chrome.action.onClicked listener and its chrome.action.setBadgeText call to
eliminate unreachable code, OR make the popup per-tab so onClicked can fire by
changing how you call chrome.action.setPopup (use the tabId form when assigning
the popup for each tab when it is created or updated) and ensure you pass the
tabId used by chrome.action.setPopup so chrome.action.onClicked will be
reachable; update or remove the badge-clearing logic accordingly (the symbols to
edit are chrome.action.setPopup, chrome.action.onClicked, and
chrome.action.setBadgeText).
- Around line 115-131: The chrome.storage.onChanged listener is unnecessary and
dangerous: remove the chrome.storage.onChanged.addListener block (which calls
chrome.storage.local.set) since the codebase only uses chrome.storage.local and
re-storing every change risks corrupting keys; if cross-namespace mirroring was
intended, replace it instead with a guarded handler that checks namespace ===
"sync" and only mirrors a specific allowlist of keys. Also remove or comment out
the chrome.action.onClicked listener because chrome.action.setPopup prevents
onClicked from firing; keep only the popup or the click handler, not both (refer
to chrome.action.setPopup and chrome.action.onClicked).
In `@extension/public/manifest.json`:
- Line 19: Update extension/public/manifest.json host_permissions to include the
actual backend origin used by your fetch calls (e.g., add
"http://localhost:5000/*" or a broader pattern like "http://localhost/*" or
"http://127.0.0.1:*/") so requests from TextInput.jsx (fetch calls around lines
41, 68, 138), Answer.jsx (around lines 100, 108, 117), SidePanel.jsx (around
line 105) and Question.jsx (around line 105) are allowed; modify the
"host_permissions" array to explicitly list the backend origin(s) that match
those fetch targets.
---
Nitpick comments:
In @.gitignore:
- Around line 24-31: The .gitignore contains a redundant entry
"extension/node_modules" because the broader "node_modules/" pattern already
ignores any node_modules at any depth; remove the specific
"extension/node_modules" line so only the general "node_modules/" entry remains,
leaving other entries like "backend/credentials.json" and "venv" unchanged.
In @.prettierignore:
- Line 4: Add the minified CSS ignore pattern to .prettierignore so
vendored/generated *.min.css files are skipped like *.min.js; update the
.prettierignore file by adding the pattern "*.min.css" alongside the existing
"*.min.js" entry to prevent Prettier from reformatting minified stylesheets.
In @.prettierrc:
- Around line 1-11: Add an explicit endOfLine setting to the Prettier config:
update the .prettierrc JSON to include the "endOfLine" property set to "lf" so
that Prettier behavior is consistent across platforms and versions; modify the
existing config object (the entry with keys like "semi", "singleQuote",
"tabWidth", etc.) to add "endOfLine": "lf".
In `@eduaid_desktop/package.json`:
- Line 33: The package.json in eduaid_desktop declares "prettier": "^3.2.5"
which diverges from eduaid_web's "^3.8.1"; update the Prettier dependency string
in eduaid_desktop's package.json (the "prettier" entry) to "^3.8.1" so both
projects declare the same minimum compatible version, then run install/update
lockfile (e.g., npm install or yarn install) to refresh the lockfile.
In `@extension/public/background.js`:
- Around line 17-57: The context menu handler (chrome.contextMenus.onClicked)
and contentScript.js both write the same selectedText to chrome.storage.local,
causing a redundant double-write; remove one of these writes—either delete the
chrome.storage.local.set({ selectedText: ... }) call in the onClicked handler or
remove the write in contentScript.js (line that writes selectedText) and rely on
the remaining flow where the selection is sent via chrome.tabs.sendMessage to
contentScript.js; ensure whichever write you keep still provides the
selectedText to consumers (via storage or message) and update any dependent
logic accordingly.
- Around line 1-14: The context menu creation is executed at top-level and can
cause duplicate ID errors on service worker restart; move the two
chrome.contextMenus.create calls (the one creating id "askExtension" and the one
creating id "generateQuizFromYouTube") into a
chrome.runtime.onInstalled.addListener callback so menus are created only on
install/update, i.e., wrap both create calls inside
chrome.runtime.onInstalled.addListener(() => { ... }) and remove the top-level
creates to prevent duplicate ID collisions on reloads.
In `@extension/src/pages/previous/Previous.jsx`:
- Line 2: Replace the legacy ReactDOM.render usage: change the import from
"react-dom" to use createRoot from "react-dom/client" (replace ReactDOM import),
locate the ReactDOM.render call (ReactDOM.render(...)) and replace it by
creating a root with createRoot(container) and calling .render(...) on that root
(keep the same root container element and the same component tree), ensuring
ReactDOM references are removed and the new createRoot API is used in
Previous.jsx.
In `@LICENSE.md`:
- Line 1: Prettier is reformatting the GPLv3 license file (LICENSE.md) which
must be preserved verbatim; update the repository's formatting ignore list by
adding "LICENSE.md" to .prettierignore so Prettier skips this file. Locate the
.prettierignore file (or create it if missing) and add a line with LICENSE.md,
commit the change so the linter/formatter will no longer modify the LICENSE.md
file.
In `@package.json`:
- Line 10: Update the Prettier version in the root package.json to match the
workspace packages: change the "prettier" dependency (currently "^3.2.5") to the
newer pinned version used by extension/package.json and eduaid_web/package.json
("^3.8.1") so all subpackages resolve the same Prettier release and avoid
formatting divergences in CI.
Contributor
|
Thanks for your contribution, for now can you just add the configs, as these changes can not be reviewed. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
extension/src/pages/previous/Previous.jsx (1)
7-7:⚠️ Potential issue | 🟡 MinorInaccurate inline comment —
FaArrowRightis a right-arrow, not an "enter" icon.🔧 Proposed fix
-import { FaArrowRight } from "react-icons/fa"; // Import the enter icon +import { FaArrowRight } from "react-icons/fa"; // Right-arrow icon for quiz list items🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/src/pages/previous/Previous.jsx` at line 7, The inline comment for the import of FaArrowRight is incorrect—it's a right-arrow icon, not an "enter" icon; update the comment next to the import of FaArrowRight in Previous.jsx to a concise, accurate description such as "Import right-arrow icon" (or remove the comment entirely) so it correctly reflects the FaArrowRight symbol.
🧹 Nitpick comments (2)
.gitignore (1)
29-30: LGTM — broadeningnode_modules/is the right call.Replacing the scoped
extension/node_moduleswith the unanchorednode_modules/correctly covers all subprojectnode_modulesdirectories (root,eduaid_web,eduaid_desktop,extension) that now exist after this PR's tooling additions.One operational note: if any files inside
extension/node_moduleswere ever committed to the repo (i.e., are still tracked in the git index), the new pattern won't silently remove them. Verify with:#!/bin/bash # Check whether any node_modules content is currently tracked in the index. git ls-files | grep 'node_modules/'If output is non-empty, run
git rm -r --cached node_modules/(per subproject as needed) and commit the result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 29 - 30, Update the repo to ensure no node_modules files remain tracked after broadening the .gitignore pattern: run a check for tracked node_modules entries (e.g., via git ls-files | grep 'node_modules/') and if any results appear, remove them from the index using git rm -r --cached node_modules/ (repeat per subproject as needed) and commit the change so the new unanchored node_modules/ entry in .gitignore takes effect.extension/src/pages/previous/Previous.jsx (1)
67-82: Consider a stablekeyfor list items.Using array index as
keyis an antipattern when items can be added/removed individually — React may reuse stale component state across renders. The risk here is low since the entire list is replaced atomically, but if individual quiz deletion is added later, this will silently cause stale-render bugs. A stable key derived from quiz content (e.g.,quiz.date + quiz.difficulty) would be safer.♻️ Proposed refactor for stable key
- {quizzes.map((quiz, index) => ( - <li - key={index} + {quizzes.map((quiz) => ( + <li + key={`${quiz.date}-${quiz.difficulty}-${quiz.numQuestions}`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/src/pages/previous/Previous.jsx` around lines 67 - 82, The list items in the Previous component use the array index as the React key which can cause stale-component state; change the key on the mapped <li> in the quizzes.map(...) to a stable identifier (e.g., use quiz.id if present, otherwise combine stable fields like quiz.date + '-' + quiz.difficulty) so each quiz item has a unique, deterministic key; update the key prop where quizzes are rendered and leave handleQuizClick and the surrounding JSX 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 `@extension/public/background.js`:
- Around line 24-27: Remove the programmatic re-injection of contentScript.js:
delete the chrome.scripting.executeScript call (the block that calls
chrome.scripting.executeScript with files: ["contentScript.js"]) since manifest
already injects contentScript.js and multiple injections duplicate the
chrome.runtime.onMessage listener and double-invoke generateQuestions() /
chrome.storage.local.set; alternatively, if you prefer to keep injection logic,
guard it by checking a page-level flag set by the content script (e.g.,
window.contentScriptLoaded) similar to window.youtubeScriptLoaded before calling
executeScript. Also replace any setTimeout-based badge clear logic (the code
that calls setTimeout to clear badges) with the Chrome Alarms API to schedule
badge-clearing from the service worker and handle the alarm in onAlarm to call
chrome.action.setBadgeText/clear.
- Around line 50-52: Replace the unreliable setTimeout-based badge clear with an
MV3-friendly approach: remove the setTimeout(... chrome.action.setBadgeText({
text: "" })) call and either (a) for delays >=30s use chrome.alarms.create with
a unique alarm name and handle badge clearing in a top-level
chrome.alarms.onAlarm listener that calls chrome.action.setBadgeText, or (b) for
short (<30s) delays keep the immediate clear only when you can guarantee the
service worker stays active or switch to an event-driven trigger (e.g.,
chrome.storage.onChanged or another top-level event) that calls
chrome.action.setBadgeText; ensure the alarm listener is registered at module
scope and reference the same alarm name when creating and handling it.
In `@extension/public/manifest.json`:
- Line 19: Remove the unused host permission entry from the manifest by editing
the "host_permissions" array in manifest.json (the "host_permissions" property)
to drop "http://127.0.0.1:8000/*" and leave only the necessary
"http://localhost:5000/*" entry, ensuring the JSON remains valid
(commas/spacing) after removal.
In `@extension/public/youtubeContentScript.js`:
- Around line 16-31: The fetch block that calls /getTranscript should check
response.ok before calling response.json(): in the promise chain where
fetch(...) is used (the block that later calls generateQuestions and
chrome.storage.local.set), first inspect response.ok and if false read
response.text() (or throw an Error with response.status and statusText) so you
handle non-2xx responses explicitly; then only call response.json() for OK
responses, log the status/error with console.error when non-ok, and ensure the
.catch still handles network/parsing errors.
In `@extension/src/pages/previous/Previous.jsx`:
- Around line 108-110: Guard against a null DOM container before calling
createRoot: check the value returned by document.getElementById("root") (the
variable container) and only call createRoot(container) and
root.render(<Previous />) when container is a valid HTMLElement; otherwise
handle the missing element (e.g., log an error or early return). Update the code
paths that reference container/createRoot/root.render to perform this null check
so createRoot is never invoked with a null target.
---
Outside diff comments:
In `@extension/src/pages/previous/Previous.jsx`:
- Line 7: The inline comment for the import of FaArrowRight is incorrect—it's a
right-arrow icon, not an "enter" icon; update the comment next to the import of
FaArrowRight in Previous.jsx to a concise, accurate description such as "Import
right-arrow icon" (or remove the comment entirely) so it correctly reflects the
FaArrowRight symbol.
---
Nitpick comments:
In @.gitignore:
- Around line 29-30: Update the repo to ensure no node_modules files remain
tracked after broadening the .gitignore pattern: run a check for tracked
node_modules entries (e.g., via git ls-files | grep 'node_modules/') and if any
results appear, remove them from the index using git rm -r --cached
node_modules/ (repeat per subproject as needed) and commit the change so the new
unanchored node_modules/ entry in .gitignore takes effect.
In `@extension/src/pages/previous/Previous.jsx`:
- Around line 67-82: The list items in the Previous component use the array
index as the React key which can cause stale-component state; change the key on
the mapped <li> in the quizzes.map(...) to a stable identifier (e.g., use
quiz.id if present, otherwise combine stable fields like quiz.date + '-' +
quiz.difficulty) so each quiz item has a unique, deterministic key; update the
key prop where quizzes are rendered and leave handleQuizClick and the
surrounding JSX intact.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/prettier.yml.gitignore.prettierignore.prettierrceduaid_desktop/package.jsoneduaid_web/package.jsoneduaid_web/src/index.cssextension/package.jsonextension/public/background.jsextension/public/manifest.jsonextension/public/youtubeContentScript.jsextension/src/pages/previous/Previous.jsxpackage.json
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (6)
- eduaid_desktop/package.json
- extension/package.json
- eduaid_web/package.json
- .github/workflows/prettier.yml
- .prettierrc
- eduaid_web/src/index.css
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
extension/src/pages/previous/Previous.jsx (1)
108-114:console.errorproduces a silent failure — preferthrowfor the missing-root branch.The null guard correctly addresses the past concern, but
console.error+ implicit return means the popup renders completely blank with no visible feedback to the developer or any error boundary. In contrast,throw new Error(...)immediately surfaces as an uncaught exception in DevTools, making the mis-configuration obvious.🛡️ Proposed fix — throw instead of silently logging
const container = document.getElementById("root"); -if (!container) { - console.error("Root container not found — cannot mount <Previous />"); -} else { - const root = createRoot(container); - root.render(<Previous />); -} +if (!container) throw new Error("Root container not found — cannot mount <Previous />"); +const root = createRoot(container); +root.render(<Previous />);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/src/pages/previous/Previous.jsx` around lines 108 - 114, Replace the silent failure in the null-check for the root container: inside the block that currently logs via console.error("Root container not found — cannot mount <Previous />"), throw a new Error with that message instead so the missing container surfaces as an uncaught exception; update the guard around document.getElementById("root") (symbols: container, createRoot, root.render, <Previous />) to throw rather than return silently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extension/public/background.js`:
- Line 49: The chrome.alarms.create call using delayInMinutes: 1/30 is below
Chrome's 0.5-minute minimum and is silently clamped (so the "// ~2s" comment is
wrong); update the badge-clear logic in the chrome.alarms.create calls (the one
at the current location and the matching YouTube handler) to either use
delayInMinutes: 0.5 (Option A) or replace the alarm-based approach with an
immediate setTimeout of ~2000ms inside the click/message handlers to clear the
badge (Option B), and correct the inaccurate inline comment near the top (the
comment stating minimum is 0) -- also update or keep the onAlarm listener (lines
20–24) only if you retain alarm-based clearing. Ensure you modify the
chrome.alarms.create invocations and the corresponding onClicked/onMessage
handlers accordingly.
---
Nitpick comments:
In `@extension/src/pages/previous/Previous.jsx`:
- Around line 108-114: Replace the silent failure in the null-check for the root
container: inside the block that currently logs via console.error("Root
container not found — cannot mount <Previous />"), throw a new Error with that
message instead so the missing container surfaces as an uncaught exception;
update the guard around document.getElementById("root") (symbols: container,
createRoot, root.render, <Previous />) to throw rather than return silently.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
extension/public/background.jsextension/public/manifest.jsonextension/public/youtubeContentScript.jsextension/src/pages/previous/Previous.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- extension/public/manifest.json
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
extension/public/background.js (1)
47-49:⚠️ Potential issue | 🟠 MajorFix alarm-delay mismatch and incorrect minimum-delay comment.
Line 48 says the minimum is
0, but for MV3 alarms the effective minimum is0.5minutes (30s). Also, Line 77 uses1 / 30(~2s), which will be clamped, so behavior won’t match the comment/intent.Suggested patch
- // Schedule badge clear via Alarms API (MV3-safe; delayInMinutes minimum - // is 0 which fires as soon as possible, ~immediately for short delays). + // Schedule badge clear via Alarms API (MV3-safe; minimum is 0.5 min / 30s). chrome.alarms.create("clearBadge", { delayInMinutes: 0.5 }); // 30s (API minimum) ... - chrome.alarms.create("clearBadge", { delayInMinutes: 1 / 30 }); // ~2s + chrome.alarms.create("clearBadge", { delayInMinutes: 0.5 }); // 30s (API minimum)Chrome Extensions MV3 alarms API: what is the minimum effective delayInMinutes, and what happens when delayInMinutes is set below that minimum?Also applies to: 76-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/public/background.js` around lines 47 - 49, The alarm delay comment and values are incorrect: MV3 clamps delayInMinutes to a minimum of 0.5 (30s), so update the chrome.alarms.create("clearBadge", { delayInMinutes: 0.5 }) comment to state the effective minimum is 0.5 minutes (30s) and change any other uses where delayInMinutes is set below 0.5 (e.g., the spot using 1/30 ≈ 0.033) to use 0.5 or a configurable constant; search for all chrome.alarms.create calls and ensure both the numeric argument and inline comment reflect the 0.5-minute minimum and that behavior is not relying on clamped sub-0.5 values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extension/public/background.js`:
- Around line 87-94: The callback inside chrome.storage.local.set currently logs
the raw selected text (request.text) which may leak sensitive user data; update
the chrome.storage.local.set callback in background.js to stop logging the raw
payload—either remove the console.log line or replace it with a non-sensitive
message (e.g., "[REDACTED]" or only the text length/metadata) and keep the
sendResponse({ status: "success" }) behavior unchanged so storage success is
still signaled.
---
Duplicate comments:
In `@extension/public/background.js`:
- Around line 47-49: The alarm delay comment and values are incorrect: MV3
clamps delayInMinutes to a minimum of 0.5 (30s), so update the
chrome.alarms.create("clearBadge", { delayInMinutes: 0.5 }) comment to state the
effective minimum is 0.5 minutes (30s) and change any other uses where
delayInMinutes is set below 0.5 (e.g., the spot using 1/30 ≈ 0.033) to use 0.5
or a configurable constant; search for all chrome.alarms.create calls and ensure
both the numeric argument and inline comment reflect the 0.5-minute minimum and
that behavior is not relying on clamped sub-0.5 values.
extension/public/background.js
Outdated
Comment on lines
+87
to
+94
| chrome.storage.local.set( | ||
| { | ||
| selectedText: request.text, | ||
| }, | ||
| () => { | ||
| console.log("Text saved to storage:", request.text); | ||
| sendResponse({ status: "success" }); | ||
| }); | ||
| return true; // Required for async sendResponse | ||
| } | ||
| }); | ||
|
|
||
| //Clear badge when popup is opened | ||
| chrome.action.onClicked.addListener(() => { | ||
| chrome.action.setBadgeText({ text: "" }); | ||
| }); | ||
|
|
||
| chrome.storage.onChanged.addListener((changes, namespace) => { | ||
| for (let [key, { oldValue, newValue }] of Object.entries(changes)) { | ||
| console.log( | ||
| `Storage key "${key}" in namespace "${namespace}" changed.`, | ||
| `Old value was "${oldValue}", new value is "${newValue}".` | ||
| ); | ||
|
|
||
| // Store the key-value pair in local storage | ||
| chrome.storage.local.set({ [key]: newValue }, () => { | ||
| if (chrome.runtime.lastError) { | ||
| console.error("Error storing data:", chrome.runtime.lastError); | ||
| } else { | ||
| console.log(`Stored key-value pair: { ${key}: ${newValue} }`); | ||
| } | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid logging raw selected text payloads.
request.text can contain sensitive user content; logging it increases privacy risk without clear operational value.
Suggested patch
- console.log("Text saved to storage:", request.text);
+ console.log("Text saved to storage");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extension/public/background.js` around lines 87 - 94, The callback inside
chrome.storage.local.set currently logs the raw selected text (request.text)
which may leak sensitive user data; update the chrome.storage.local.set callback
in background.js to stop logging the raw payload—either remove the console.log
line or replace it with a non-sensitive message (e.g., "[REDACTED]" or only the
text length/metadata) and keep the sendResponse({ status: "success" }) behavior
unchanged so storage success is still signaled.
- Add .prettierrc with project formatting rules - Add .prettierignore to exclude build artifacts, vendor dirs, and non-JS folders - Add root package.json with format/check-format scripts - Add GitHub Actions prettier.yml CI workflow (runs on PRs touching JS/web packages) - Add format/format:check scripts and prettier@3.8.1 devDep to extension, eduaid_web, and eduaid_desktop packages
aryash45
force-pushed
the
chore/prettier-config
branch
from
791d388 to
7b9f464
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_desktop/package.json`:
- Line 33: The Prettier devDependency is exact-pinned to "3.8.1" while other
devDependencies use caret ranges; update the "prettier" entry in package.json to
use a caret range ("^3.8.1") to match the project's versioning convention and
allow patch updates, or if exact pinning is intentional, add a brief comment or
documentation explaining the rationale for keeping "prettier" exact-pinned.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/prettier.yml.prettierignore.prettierrceduaid_desktop/package.jsoneduaid_web/package.jsonextension/package.jsonpackage.json
🚧 Files skipped from review as they are similar to previous changes (6)
- eduaid_web/package.json
- package.json
- extension/package.json
- .prettierrc
- .prettierignore
- .github/workflows/prettier.yml
Author
|
@yatikakain please check this |
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
2 participants
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.
PR Description
Closes #407
Addressed Issues:
This PR standardizes code formatting across the repository by introducing a centralized Prettier configuration. It resolves the inconsistency in code styles (indentation, quotes, etc.) found in various files.
Description of Changes:
prettieras a dev dependency.eduaid_desktop,eduaid_web, andextensionto align with the root configuration.prettier --write .on the entire codebase.npm run formatandnpm run check-formatfor easy verification.Screenshots/Recordings:
N/A - This is a code formatting update.
Additional Notes:
node_modules/is now correctly ignored in .gitignore to prevent accidental commits.npm installin subprojects (eduaid_web,eduaid_desktop) may fail due to pre-existing dependency conflicts unrelated to this PR, but the formatting is enforced globally via the rootnpm run format.Checklist
npm run check-formatpasses)Summary by CodeRabbit
Release Notes
New Features
Chores