Add Match-the-Columns Question Type#503
Open
Krishna41357 wants to merge 7 commits intoAOSSIE-Org:mainfrom
Open
Add Match-the-Columns Question Type#503Krishna41357 wants to merge 7 commits intoAOSSIE-Org:mainfrom
Krishna41357 wants to merge 7 commits intoAOSSIE-Org:mainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a Match-the-Columns feature: new backend NLP module to extract term–definition pairs, a POST API endpoint to return match-columns payload, frontend wiring to assemble and render MatchColumns entries (two-column static view, no per-item editing), and PDF worker support to render match columns with an optional answer key. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as Frontend (Output.jsx)
participant Server as Server (backend/server.py)
participant NLP as NLP Module (match_columns.py)
participant PDF as PDF Worker
User->>Frontend: choose "Match the Columns" & submit text
Frontend->>Server: POST /get_match_columns {input_text, max_questions}
Server->>NLP: generate_match_columns(payload)
NLP-->>Server: return {pairs, left_column, right_column}
Server-->>Frontend: respond with match-columns payload
Frontend->>Frontend: save entry, render two-column view
Frontend->>PDF: request PDF generation (mode)
PDF->>PDF: render MatchColumns two-column layout and Answer Key
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🧹 Nitpick comments (5)
backend/Generator/match_columns.py (3)
3-3: Unused import:sent_tokenizefrom nltk.The
sent_tokenizefunction is imported but never used. The code usesdoc.sentsfrom spaCy instead for sentence iteration.🧹 Remove unused import
import spacy import random -from nltk.tokenize import sent_tokenize🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/Generator/match_columns.py` at line 3, Remove the unused nltk import: delete the "sent_tokenize" import statement (the symbol sent_tokenize) since the module uses spaCy's doc.sents for sentence iteration; search for "from nltk.tokenize import sent_tokenize" and remove that line to eliminate the unused import and related lint warnings.
35-37: Same redundant parsing in fallback loop.Apply the same fix here to use
sent.noun_chunksdirectly instead of re-parsing.♻️ Use the existing span directly
if len(pairs) < 3: for sent in doc.sents: - sent_doc = nlp(sent.text) - for chunk in sent_doc.noun_chunks: + for chunk in sent.noun_chunks: term = chunk.text.strip()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/Generator/match_columns.py` around lines 35 - 37, The fallback loop in match_columns.py currently reparses each sentence with nlp(sent.text) (see variables sent_doc and the loop for chunk in sent_doc.noun_chunks), which is redundant; replace that reparsing by iterating over sent.noun_chunks directly (use for chunk in sent.noun_chunks) and remove the sent_doc variable to avoid unnecessary NLP calls and duplicate work.
13-16: Redundant NLP parsing on each sentence.
doc.sentsyieldsSpanobjects that already have.entsand.noun_chunksattributes from the initial parse. Re-parsing each sentence withnlp(sent.text)doubles the processing cost.♻️ Use the existing span directly
for sent in doc.sents: - sent_doc = nlp(sent.text) - - for ent in sent_doc.ents: + for ent in sent.ents: term = ent.text.strip()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/Generator/match_columns.py` around lines 13 - 16, The loop currently re-parses each sentence with nlp(sent.text) (creating sent_doc) which is redundant and costly; instead use the existing Span from doc.sents and access its attributes directly (replace usages of sent_doc.ents and sent_doc.noun_chunks with sent.ents and sent.noun_chunks) and remove the nlp(sent.text) call to avoid double parsing in match_columns.py.eduaid_web/src/pages/Output.jsx (1)
339-345: SameindexOfedge case as in pdfWorker.If
pair.definitionis not found inright_column,indexOfreturns -1, producing an invalid character.🛡️ Add defensive check
{qaPair.pairs.map((pair, i) => ( - <div key={i} className="text-[`#FFF4F4`] text-xs sm:text-sm mb-1"> - {i + 1} → {String.fromCharCode( - 65 + qaPair.right_column.indexOf(pair.definition) - )} - </div> + <div key={i} className="text-[`#FFF4F4`] text-xs sm:text-sm mb-1"> + {i + 1} → {(() => { + const idx = qaPair.right_column.indexOf(pair.definition); + return idx >= 0 ? String.fromCharCode(65 + idx) : '?'; + })()} + </div> ))}🤖 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 339 - 345, In the QA render loop in Output.jsx (the qaPair.pairs.map block), guard against qaPair.right_column.indexOf(pair.definition) returning -1 by computing the index into a variable (e.g., idx) and checking if idx === -1; when -1, render a safe fallback (e.g., '?' or an empty placeholder) instead of passing -1 to String.fromCharCode, otherwise render String.fromCharCode(65 + idx). This ensures pair.definition not found in qaPair.right_column won't produce an invalid character.eduaid_web/src/workers/pdfWorker.js (1)
147-157: Consider handling the case whereindexOfreturns -1.If
pair.definitionis not found inrightColumn(data inconsistency),indexOfreturns -1, which would produceString.fromCharCode(64)yielding@instead of a valid letter.🛡️ Add defensive check
pairs.forEach((pair, i) => { const correctIndex = rightColumn.indexOf(pair.definition); - const letter = String.fromCharCode(65 + correctIndex); + const letter = correctIndex >= 0 + ? String.fromCharCode(65 + correctIndex) + : '?'; page.drawText(`${i + 1} → ${letter}`, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eduaid_web/src/workers/pdfWorker.js` around lines 147 - 157, The forEach that maps pairs to letters uses rightColumn.indexOf(pair.definition) and passes that to String.fromCharCode without checking for -1, which yields "@" for missing entries; update the pairs.forEach block (the loop using pairs, rightColumn, pair.definition, correctIndex and String.fromCharCode) to handle correctIndex === -1 defensively — choose a clear fallback (e.g., "?" or "—"), or skip rendering and optionally log/warn about the inconsistent pair, and then use the fallback value when calling page.drawText so no invalid "@" character is produced.
🤖 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/Output.jsx`:
- Around line 123-133: The current condition uses
qaPairsFromStorage["output_mcq"] || questionType === "get_mcq" which can be true
due to stale storage even when the current questionType isn't MCQ; change the
check to require the current question type be MCQ and storage exist (e.g., use
if (questionType === "get_mcq" && qaPairsFromStorage["output_mcq"]) { ... }), so
the loop that pushes into combinedQaPairs (using qaPair.question_statement,
options, answer, context) runs only for actual MCQ requests.
---
Nitpick comments:
In `@backend/Generator/match_columns.py`:
- Line 3: Remove the unused nltk import: delete the "sent_tokenize" import
statement (the symbol sent_tokenize) since the module uses spaCy's doc.sents for
sentence iteration; search for "from nltk.tokenize import sent_tokenize" and
remove that line to eliminate the unused import and related lint warnings.
- Around line 35-37: The fallback loop in match_columns.py currently reparses
each sentence with nlp(sent.text) (see variables sent_doc and the loop for chunk
in sent_doc.noun_chunks), which is redundant; replace that reparsing by
iterating over sent.noun_chunks directly (use for chunk in sent.noun_chunks) and
remove the sent_doc variable to avoid unnecessary NLP calls and duplicate work.
- Around line 13-16: The loop currently re-parses each sentence with
nlp(sent.text) (creating sent_doc) which is redundant and costly; instead use
the existing Span from doc.sents and access its attributes directly (replace
usages of sent_doc.ents and sent_doc.noun_chunks with sent.ents and
sent.noun_chunks) and remove the nlp(sent.text) call to avoid double parsing in
match_columns.py.
In `@eduaid_web/src/pages/Output.jsx`:
- Around line 339-345: In the QA render loop in Output.jsx (the qaPair.pairs.map
block), guard against qaPair.right_column.indexOf(pair.definition) returning -1
by computing the index into a variable (e.g., idx) and checking if idx === -1;
when -1, render a safe fallback (e.g., '?' or an empty placeholder) instead of
passing -1 to String.fromCharCode, otherwise render String.fromCharCode(65 +
idx). This ensures pair.definition not found in qaPair.right_column won't
produce an invalid character.
In `@eduaid_web/src/workers/pdfWorker.js`:
- Around line 147-157: The forEach that maps pairs to letters uses
rightColumn.indexOf(pair.definition) and passes that to String.fromCharCode
without checking for -1, which yields "@" for missing entries; update the
pairs.forEach block (the loop using pairs, rightColumn, pair.definition,
correctIndex and String.fromCharCode) to handle correctIndex === -1 defensively
— choose a clear fallback (e.g., "?" or "—"), or skip rendering and optionally
log/warn about the inconsistent pair, and then use the fallback value when
calling page.drawText so no invalid "@" character is produced.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/Generator/match_columns.pybackend/server.pyeduaid_web/src/pages/Output.jsxeduaid_web/src/pages/Question_Type.jsxeduaid_web/src/workers/pdfWorker.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
eduaid_web/src/pages/Output.jsx (1)
13-13: Remove unusedpdfModestate variable.The
pdfModestate is declared but never read. The PDF mode is passed directly from button click handlers togeneratePDF(mode), making this state unnecessary.🧹 Suggested fix
- const [pdfMode, setPdfMode] = useState("questions");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eduaid_web/src/pages/Output.jsx` at line 13, Remove the unused state by deleting the useState declaration for pdfMode and its setter (const [pdfMode, setPdfMode] = useState("questions")) from Output.jsx and any associated imports if now unused; ensure that generatePDF(mode) continues to receive the mode directly from the button click handlers (leave those handlers as-is) and run tests/formatting to confirm no references to pdfMode or setPdfMode remain.
🤖 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/Output.jsx`:
- Around line 339-345: The JSX rendering of answer keys uses
String.fromCharCode(65 + qaPair.right_column.indexOf(pair.definition)) which
yields 64/"@" when indexOf returns -1; update the rendering to compute the index
first (use qaPair.right_column.indexOf(pair.definition)), check if idx >= 0, and
then render String.fromCharCode(65 + idx) else render a safe fallback like "?"
(or another sentinel). For clarity and maintainability, you can compute an
answerMap from qaPair.pairs before the JSX and reference that map in the render
instead of calling indexOf inline.
---
Nitpick comments:
In `@eduaid_web/src/pages/Output.jsx`:
- Line 13: Remove the unused state by deleting the useState declaration for
pdfMode and its setter (const [pdfMode, setPdfMode] = useState("questions"))
from Output.jsx and any associated imports if now unused; ensure that
generatePDF(mode) continues to receive the mode directly from the button click
handlers (leave those handlers as-is) and run tests/formatting to confirm no
references to pdfMode or setPdfMode remain.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
eduaid_web/src/pages/Output.jsx (2)
13-13: Unused state variablepdfMode.
pdfModeis declared but never read. The PDF mode is passed directly togeneratePDFvia the onClick handlers. Consider removing this unused state.🧹 Suggested fix
const [questionType, setQuestionType] = useState( localStorage.getItem("selectedQuestionType") ); - const [pdfMode, setPdfMode] = useState("questions"); const [editingIndex, setEditingIndex] = useState(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eduaid_web/src/pages/Output.jsx` at line 13, Remove the unused React state by deleting the declaration const [pdfMode, setPdfMode] = useState("questions") and any unused setPdfMode references in Output.jsx; instead pass the literal mode strings directly to generatePDF in the onClick handlers (e.g., generatePDF("questions") / generatePDF("answers")), or if dynamic mode was intended, replace usage with a single prop/variable that is actually read—locate the pdfMode and setPdfMode symbols and remove or replace them accordingly.
19-30: Consider React state for dropdown visibility.The dropdown uses direct DOM manipulation (
getElementById,classList.toggle). A React state-based approach would be more idiomatic and easier to test:♻️ Suggested refactor using React state
+ const [pdfDropdownOpen, setPdfDropdownOpen] = useState(false); useEffect(() => { const handleClickOutside = (event) => { - const dropdown = document.getElementById('pdfDropdown'); - if (dropdown && !dropdown.contains(event.target) && - !event.target.closest('button')) { - dropdown.classList.add('hidden'); + if (pdfDropdownOpen && !event.target.closest('.pdf-dropdown-container')) { + setPdfDropdownOpen(false); } }; // ... }, [pdfDropdownOpen]); // In JSX: - <div className="relative w-full sm:w-auto"> + <div className="relative w-full sm:w-auto pdf-dropdown-container"> <button - onClick={() => document.getElementById('pdfDropdown').classList.toggle('hidden')} + onClick={() => setPdfDropdownOpen(!pdfDropdownOpen)} > Generate PDF </button> <div - id="pdfDropdown" - className="hidden absolute ..." + className={`${pdfDropdownOpen ? '' : 'hidden'} absolute ...`} >Also applies to: 448-458
🤖 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 19 - 30, Replace direct DOM manipulation in handleClickOutside and any code using document.getElementById('pdfDropdown') or classList toggles with React state: introduce a state boolean (e.g., isPdfDropdownOpen) and setters, render the dropdown conditionally based on that state, and toggle it via event handlers instead of classList. Use a ref (e.g., pdfDropdownRef) attached to the dropdown element and in the existing useEffect detect outside clicks by comparing event.target to pdfDropdownRef.current, calling setIsPdfDropdownOpen(false) on outside clicks and cleaning up the listener in the cleanup function; apply the same ref/state refactor to the other dropdown usage referenced in the file (the code block around lines 448-458).
🤖 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/Output.jsx`:
- Around line 99-121: The combinedQaPairs population unconditionally processes
qaPairsFromStorage["output_boolq"] and ["output_mcq"], causing stale MCQ/Boolean
entries to be included when the current questionType is different; update the
logic in the block that builds combinedQaPairs (references: combinedQaPairs,
qaPairsFromStorage, output_boolq, output_mcq) to only run these loops when the
current questionType is appropriate (e.g., when questionType === "get_all" or
questionType === "Boolean" for output_boolq, and when questionType === "get_all"
or questionType === "MCQ" for output_mcq) so old data is not mixed into new
outputs.
- Around line 323-332: The mapping over qaPair.left_column assumes
qaPair.right_column has the same length, which can yield undefined; update the
render inside the map for qaPair.left_column to safely access the matching right
value (use optional chaining and a default/fallback like an empty string or
placeholder) or compute an index-based safe value (e.g., const right =
qaPair.right_column?.[i] ?? '') before rendering; refer to qaPair, left_column,
right_column and the map callback to locate where to replace the direct
qaPair.right_column[i] access with the defensive expression or fallback.
---
Nitpick comments:
In `@eduaid_web/src/pages/Output.jsx`:
- Line 13: Remove the unused React state by deleting the declaration const
[pdfMode, setPdfMode] = useState("questions") and any unused setPdfMode
references in Output.jsx; instead pass the literal mode strings directly to
generatePDF in the onClick handlers (e.g., generatePDF("questions") /
generatePDF("answers")), or if dynamic mode was intended, replace usage with a
single prop/variable that is actually read—locate the pdfMode and setPdfMode
symbols and remove or replace them accordingly.
- Around line 19-30: Replace direct DOM manipulation in handleClickOutside and
any code using document.getElementById('pdfDropdown') or classList toggles with
React state: introduce a state boolean (e.g., isPdfDropdownOpen) and setters,
render the dropdown conditionally based on that state, and toggle it via event
handlers instead of classList. Use a ref (e.g., pdfDropdownRef) attached to the
dropdown element and in the existing useEffect detect outside clicks by
comparing event.target to pdfDropdownRef.current, calling
setIsPdfDropdownOpen(false) on outside clicks and cleaning up the listener in
the cleanup function; apply the same ref/state refactor to the other dropdown
usage referenced in the file (the code block around lines 448-458).
eduaid_web/src/pages/Output.jsx
Outdated
Comment on lines
+323
to
+332
| {qaPair.left_column.map((term, i) => ( | ||
| <div key={i} className="flex justify-between gap-4 mb-2 px-2"> | ||
| <div className="text-[#FFF4F4] text-sm flex-1 bg-[#1a1a2e] p-2 rounded border border-gray-700"> | ||
| {i + 1}. {term} | ||
| </div> | ||
| <div className="text-[#FFF4F4] text-sm flex-1 bg-[#1a1a2e] p-2 rounded border border-gray-700"> | ||
| {String.fromCharCode(65 + i)}. {qaPair.right_column[i]} | ||
| </div> | ||
| </div> | ||
| <textarea | ||
| className="w-full bg-[#1a1a2e] text-[#FFF4F4] text-sm sm:text-base p-2 rounded border border-gray-600 focus:border-[#7600F2] focus:outline-none resize-none" | ||
| rows="2" | ||
| value={editedAnswer} | ||
| onChange={(e) => setEditedAnswer(e.target.value)} | ||
| /> | ||
|
|
||
| {editedOptions && editedOptions.length > 0 && ( | ||
| <div className="mt-3"> | ||
| <div className="text-[#E4E4E4] text-xs sm:text-sm mb-2"> | ||
| Edit Options | ||
| ))} |
There was a problem hiding this comment.
Add defensive check for right_column array access.
Line 329 accesses qaPair.right_column[i] assuming it has the same length as left_column. If data is malformed and right_column is shorter, this renders undefined.
🛡️ Suggested defensive fix
{qaPair.left_column.map((term, i) => (
<div key={i} className="flex justify-between gap-4 mb-2 px-2">
<div className="text-[`#FFF4F4`] text-sm flex-1 bg-[`#1a1a2e`] p-2 rounded border border-gray-700">
{i + 1}. {term}
</div>
<div className="text-[`#FFF4F4`] text-sm flex-1 bg-[`#1a1a2e`] p-2 rounded border border-gray-700">
- {String.fromCharCode(65 + i)}. {qaPair.right_column[i]}
+ {String.fromCharCode(65 + i)}. {qaPair.right_column[i] ?? "—"}
</div>
</div>
))}🤖 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 323 - 332, The mapping over
qaPair.left_column assumes qaPair.right_column has the same length, which can
yield undefined; update the render inside the map for qaPair.left_column to
safely access the matching right value (use optional chaining and a
default/fallback like an empty string or placeholder) or compute an index-based
safe value (e.g., const right = qaPair.right_column?.[i] ?? '') before
rendering; refer to qaPair, left_column, right_column and the map callback to
locate where to replace the direct qaPair.right_column[i] access with the
defensive expression or fallback.
There was a problem hiding this comment.
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)
99-124:⚠️ Potential issue | 🟠 MajorValidate storage payload types before iterating.
forEach/mapon Line 100, Line 112, Line 124, Line 141, Line 146, and Line 153 assume array shapes fromlocalStorage. If payload schema is stale/corrupted, this will throw and break the page.🛡️ Proposed hardening
useEffect(() => { const qaPairsFromStorage = JSON.parse(localStorage.getItem("qaPairs")) || {}; if (qaPairsFromStorage) { const combinedQaPairs = []; + const outputQuestions = qaPairsFromStorage["output"]; + const boolQuestions = qaPairsFromStorage["output_boolq"]?.["Boolean_Questions"]; + const mcqQuestions = qaPairsFromStorage["output_mcq"]?.["questions"]; - if (questionType === "get_problems" && qaPairsFromStorage["output_boolq"]) { - qaPairsFromStorage["output_boolq"]["Boolean_Questions"].forEach( + if (questionType === "get_problems" && Array.isArray(boolQuestions)) { + boolQuestions.forEach( (question) => { combinedQaPairs.push({ question, question_type: "Boolean", context: qaPairsFromStorage["output_boolq"]["Text"], }); } ); } - if (questionType === "get_problems" && qaPairsFromStorage["output_mcq"]) { - qaPairsFromStorage["output_mcq"]["questions"].forEach((qaPair) => { + if (questionType === "get_problems" && Array.isArray(mcqQuestions)) { + mcqQuestions.forEach((qaPair) => { combinedQaPairs.push({ question: qaPair.question_statement, question_type: "MCQ", options: qaPair.options, answer: qaPair.answer, context: qaPair.context, }); }); } - if (questionType === "get_mcq" && qaPairsFromStorage["output"]) { - qaPairsFromStorage["output"].forEach((qaPair) => { + if (questionType === "get_mcq" && Array.isArray(outputQuestions)) { + outputQuestions.forEach((qaPair) => { combinedQaPairs.push({ question: qaPair.question_statement, question_type: "MCQ", options: qaPair.options, answer: qaPair.answer, context: qaPair.context, }); }); } else if (questionType === "get_match_columns") { - const pairs = qaPairsFromStorage["pairs"] || []; - const shuffledRight = qaPairsFromStorage["right_column"] || []; + const pairs = Array.isArray(qaPairsFromStorage["pairs"]) + ? qaPairsFromStorage["pairs"] + : []; + const shuffledRight = Array.isArray(qaPairsFromStorage["right_column"]) + ? qaPairsFromStorage["right_column"] + : []; if (pairs.length > 0) { combinedQaPairs.push({ question_type: "MatchColumns", pairs: pairs, left_column: pairs.map((p) => p.term), right_column: shuffledRight, }); } - } else if (questionType === "get_boolq" && qaPairsFromStorage["output"]) { - qaPairsFromStorage["output"].forEach((qaPair) => { + } else if (questionType === "get_boolq" && Array.isArray(outputQuestions)) { + outputQuestions.forEach((qaPair) => { combinedQaPairs.push({ question: qaPair, question_type: "Boolean", }); }); - } else if (qaPairsFromStorage["output"] && questionType !== "get_mcq") { - qaPairsFromStorage["output"].forEach((qaPair) => { + } else if (Array.isArray(outputQuestions) && questionType !== "get_mcq") { + outputQuestions.forEach((qaPair) => { combinedQaPairs.push({ question: qaPair.question || qaPair.question_statement || qaPair.Question, options: qaPair.options, answer: qaPair.answer || qaPair.Answer, context: qaPair.context, question_type: "Short", }); }); }Also applies to: 134-154
🤖 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 99 - 124, The code assumes qaPairsFromStorage entries are arrays and calls forEach directly, which can throw on stale/corrupted payloads; update the blocks that reference qaPairsFromStorage (keys: "output_boolq" -> "Boolean_Questions", "output_mcq" -> "questions", "output", and any other output_* arrays) to first validate types with Array.isArray(...) (or coerce with const arr = Array.isArray(x) ? x : []) before iterating, and skip/return early when required fields are missing (e.g., ensure qaPairsFromStorage["output_boolq"] && Array.isArray(qaPairsFromStorage["output_boolq"]["Boolean_Questions"]) before calling forEach), updating the code around the questionType checks and the combinedQaPairs pushes accordingly.
20-24:⚠️ Potential issue | 🟡 MinorOutside-click logic is too broad for buttons.
On Line 23,
!event.target.closest('button')treats every button as “inside,” so the PDF menu won’t close when users click unrelated buttons (e.g., Shuffle/Edit). Restrict this exemption to the PDF toggle button only.🎯 Proposed fix
useEffect(() => { const handleClickOutside = (event) => { - const dropdown = document.getElementById('pdfDropdown'); - if (dropdown && !dropdown.contains(event.target) && - !event.target.closest('button')) { + const dropdown = document.getElementById('pdfDropdown'); + const isInsideDropdown = dropdown?.contains(event.target); + const isToggleButton = event.target.closest?.('#pdfDropdownToggle'); + if (dropdown && !isInsideDropdown && !isToggleButton) { dropdown.classList.add('hidden'); } };<button + id="pdfDropdownToggle" className="bg-[`#518E8E`] items-center flex gap-1 w-full sm:w-auto font-semibold text-white px-4 sm:px-6 py-3 sm:py-2 rounded-xl text-sm sm:text-base hover:bg-[`#3a6b6b`] transition-colors justify-center" onClick={() => document.getElementById('pdfDropdown').classList.toggle('hidden')} >Also applies to: 440-443
🤖 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 20 - 24, The outside-click handler in handleClickOutside is too permissive because it exempts all buttons via event.target.closest('button'); change that check to only exempt the PDF toggle button by identifying the PDF toggle (e.g., its id like 'pdfToggle' or a data attribute like 'data-pdf-toggle') and replace the generic closest('button') test with event.target.closest('#pdfToggle') or event.target.closest('[data-pdf-toggle]') so unrelated buttons (Shuffle/Edit) will allow the dropdown (pdfDropdown) to close; apply the same change for the other occurrence around lines 440-443.
199-209:⚠️ Potential issue | 🟡 MinorClean up download URL and guard async DOM access.
Line 202 creates an object URL but never revokes it. Also, Line 208 can throw if the element is unavailable when the worker returns.
🧹 Proposed fix
worker.onmessage = (e) => { const blob = new Blob([e.data], { type: 'application/pdf' }); const link = document.createElement('a'); - link.href = URL.createObjectURL(blob); + const objectUrl = URL.createObjectURL(blob); + link.href = objectUrl; link.download = "generated_questions.pdf"; document.body.appendChild(link); link.click(); document.body.removeChild(link); + URL.revokeObjectURL(objectUrl); - document.getElementById('pdfDropdown').classList.add('hidden'); + document.getElementById('pdfDropdown')?.classList.add('hidden'); 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 199 - 209, The current worker.onmessage handler creates an object URL with URL.createObjectURL and manipulates DOM nodes without guarding for presence; fix it by storing the object URL returned by URL.createObjectURL(blob) and revoking it after initiating the download (e.g., call URL.revokeObjectURL(objectUrl) after link.click(), possibly inside a short setTimeout) to avoid leaking blobs, and protect the DOM access to the dropdown by checking document.getElementById('pdfDropdown') exists before calling .classList.add('hidden'); keep the existing cleanup of the temporary link (removeChild) and ensure worker.terminate() still runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@eduaid_web/src/pages/Output.jsx`:
- Around line 99-124: The code assumes qaPairsFromStorage entries are arrays and
calls forEach directly, which can throw on stale/corrupted payloads; update the
blocks that reference qaPairsFromStorage (keys: "output_boolq" ->
"Boolean_Questions", "output_mcq" -> "questions", "output", and any other
output_* arrays) to first validate types with Array.isArray(...) (or coerce with
const arr = Array.isArray(x) ? x : []) before iterating, and skip/return early
when required fields are missing (e.g., ensure
qaPairsFromStorage["output_boolq"] &&
Array.isArray(qaPairsFromStorage["output_boolq"]["Boolean_Questions"]) before
calling forEach), updating the code around the questionType checks and the
combinedQaPairs pushes accordingly.
- Around line 20-24: The outside-click handler in handleClickOutside is too
permissive because it exempts all buttons via event.target.closest('button');
change that check to only exempt the PDF toggle button by identifying the PDF
toggle (e.g., its id like 'pdfToggle' or a data attribute like
'data-pdf-toggle') and replace the generic closest('button') test with
event.target.closest('#pdfToggle') or event.target.closest('[data-pdf-toggle]')
so unrelated buttons (Shuffle/Edit) will allow the dropdown (pdfDropdown) to
close; apply the same change for the other occurrence around lines 440-443.
- Around line 199-209: The current worker.onmessage handler creates an object
URL with URL.createObjectURL and manipulates DOM nodes without guarding for
presence; fix it by storing the object URL returned by URL.createObjectURL(blob)
and revoking it after initiating the download (e.g., call
URL.revokeObjectURL(objectUrl) after link.click(), possibly inside a short
setTimeout) to avoid leaking blobs, and protect the DOM access to the dropdown
by checking document.getElementById('pdfDropdown') exists before calling
.classList.add('hidden'); keep the existing cleanup of the temporary link
(removeChild) and ensure worker.terminate() still runs.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
eduaid_web/src/pages/Output.jsx (1)
96-98: Redundant truthy check due to default value.The
|| {}on line 96 ensuresqaPairsFromStorageis always truthy (at minimum an empty object), making theif (qaPairsFromStorage)check on line 97 always evaluate totrue.♻️ Optional cleanup
const qaPairsFromStorage = JSON.parse(localStorage.getItem("qaPairs")) || {}; - if (qaPairsFromStorage) { - const combinedQaPairs = []; + const combinedQaPairs = []; // ... rest of the logic - }🤖 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 96 - 98, The truthy guard is redundant because qaPairsFromStorage is set with JSON.parse(... ) || {}, so the subsequent if (qaPairsFromStorage) always passes; remove the fallback "|| {}" from the JSON.parse assignment so qaPairsFromStorage can be null when no storage exists, making the if (qaPairsFromStorage) check in Output.jsx meaningful (or, alternatively, replace the if check with a structural check like Object.keys(qaPairsFromStorage).length > 0 if you prefer to keep the default {}). Ensure you update references to qaPairsFromStorage and combinedQaPairs accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@eduaid_web/src/pages/Output.jsx`:
- Around line 96-98: The truthy guard is redundant because qaPairsFromStorage is
set with JSON.parse(... ) || {}, so the subsequent if (qaPairsFromStorage)
always passes; remove the fallback "|| {}" from the JSON.parse assignment so
qaPairsFromStorage can be null when no storage exists, making the if
(qaPairsFromStorage) check in Output.jsx meaningful (or, alternatively, replace
the if check with a structural check like Object.keys(qaPairsFromStorage).length
> 0 if you prefer to keep the default {}). Ensure you update references to
qaPairsFromStorage and combinedQaPairs accordingly.
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.
Closes #217
What's Changed
Added a new Match-the-Columns question type to EduAid as discussed in #217.
Users can now select "Match the Columns" from the question type screen, paste their study material, and get a two-column quiz where terms on the left need to be matched with their definitions on the right. The right column is shuffled to make it an actual challenge. An answer key is shown at the bottom for self-checking.
Changes
Backend
backend/Generator/match_columns.pywhich extracts key terms and their context sentences from input text using spaCy NER, with a noun chunk fallback for texts with fewer named entities. Duplicate definitions are filtered out to ensure each row in the quiz is unique./get_match_columnsPOST endpoint inserver.pythat accepts input text and max question count, and returns the term pairs, left column, and shuffled right column.Frontend
Question_Type.jsxOutput.jsxto render the two-column table layout with an answer key when this question type is selected. The Edit button is hidden for this type since the output is a single unified table rather than individual questions.PDF Export
pdfWorker.js, including proper two-column layout rendering and answer key generation.Screenshots
Testing
Verified the
/get_match_columnsendpoint locally using a test script. The endpoint correctly extracts unique term-definition pairs, shuffles the right column, and returns a valid answer key mapping.Endpoint working!
LEFT COLUMN (Terms):
CORRECT PAIRS (Answer Key):
1 → E | Photosynthesis
2 → D | Mitochondria
3 → C | Chlorophyll
4 → A | DNA
Summary by CodeRabbit
New Features
UI Improvements