Conversation
📝 WalkthroughWalkthroughThe changes standardize the Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Web as Web/Extension Client
participant Backend as Backend Server
participant Browser
User->>Web: Click "Generate Google Form"
Web->>Web: Validate qa_pairs input
Web->>Backend: POST /generate_gform (qa_pairs list)
alt Invalid Input
Backend-->>Web: HTTP 400 (error JSON)
Web-->>User: Display error
else Valid Input
Backend->>Backend: Process QA pairs
Backend->>Backend: Generate Form (responderUri)
Backend-->>Web: JSON {form_link, edit_link}
Web->>Web: Parse response (backward-compatible)
alt URL Present & Valid
Web->>Browser: window.open(form_link)
Browser-->>User: Open Google Form
else URL Missing/Invalid
Web-->>User: Log error & display message
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
There was a problem hiding this comment.
Pull request overview
Normalizes the /generate_gform API response contract in the Flask backend and updates web/extension consumers to be backward-compatible with both the new object shape and the legacy string response, improving reliability of the Google Form generation flow.
Changes:
- Backend: return
{ form_link, edit_link }, remove server-side browser-opening side effect, and validateqa_pairsis a list (400 otherwise). - Clients (web + extension): accept either
{form_link: ...}or legacy string response and avoid opening when URL is missing. - Web: harden
qaPairslocalStorage parsing and array-shape handling when assemblingqaPairs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| extension/src/pages/question/SidePanel.jsx | Adds backward-compatible parsing of /generate_gform response before opening the form link. |
| extension/src/pages/question/Question.jsx | Adds backward-compatible parsing of /generate_gform response before opening the form link. |
| eduaid_web/src/pages/Output.jsx | Hardens localStorage parsing + normalizes QA arrays; adds backward-compatible parsing for form link opening. |
| backend/server.py | Normalizes /generate_gform JSON response, adds qa_pairs type guard, and removes webbrowser side effect. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const formUrl = | ||
| (result && result.form_link) || | ||
| (typeof result === "string" ? result : null); | ||
|
|
||
| if (!formUrl) { | ||
| console.error("Google Form URL missing in API response", result); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The response-shape normalization logic is duplicated here (and in other clients). To avoid the extension/web drifting on future contract tweaks (e.g., adding edit_link or changing legacy handling), consider extracting a small helper (e.g., getGFormLinkFromResponse(result)) and reusing it across the three call sites.
| return; | ||
| } | ||
|
|
||
| window.open(formUrl, "_blank"); |
There was a problem hiding this comment.
window.open(formUrl, "_blank") can allow reverse-tabnabbing via window.opener. Prefer opening with noopener,noreferrer (or explicitly nulling opener) so the newly opened page can't navigate the extension page.
| window.open(formUrl, "_blank"); | |
| const newWindow = window.open(formUrl, "_blank", "noopener,noreferrer"); | |
| if (newWindow) { | |
| newWindow.opener = null; | |
| } |
| const formUrl = | ||
| (result && result.form_link) || | ||
| (typeof result === "string" ? result : null); | ||
|
|
||
| if (!formUrl) { | ||
| console.error("Google Form URL missing in API response", result); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The legacy/new response handling is repeated across multiple components. Consider factoring this into a shared helper so any future API contract adjustments (e.g., preferring edit_link in some flows) only need to be made once.
| return; | ||
| } | ||
|
|
||
| window.open(formUrl, "_blank"); |
There was a problem hiding this comment.
window.open(..., "_blank") should be opened with noopener/noreferrer (or newWindow.opener = null) to prevent reverse-tabnabbing from a potentially attacker-controlled URL.
| window.open(formUrl, "_blank"); | |
| const newWindow = window.open(formUrl, "_blank", "noopener,noreferrer"); | |
| if (newWindow) { | |
| newWindow.opener = null; | |
| } |
| const formUrl = | ||
| (result && result.form_link) || | ||
| (typeof result === "string" ? result : null); | ||
|
|
||
| if (!formUrl) { | ||
| console.error("Google Form URL missing in API response", result); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
This response parsing pattern is now duplicated in multiple clients. Consider extracting it into a small utility (web-side) so the /generate_gform response contract is handled consistently and changes are easier to roll out.
| return; | ||
| } | ||
|
|
||
| window.open(formUrl, "_blank"); |
There was a problem hiding this comment.
Opening a URL in a new tab with target=_blank should include noopener/noreferrer (or opener = null) to avoid reverse-tabnabbing. This is especially relevant since the URL comes from an API response.
| window.open(formUrl, "_blank"); | |
| const newWindow = window.open(formUrl, "_blank", "noopener,noreferrer"); | |
| if (newWindow) { | |
| newWindow.opener = null; | |
| } |
| data = request.get_json() or {} | ||
| qa_pairs = data.get("qa_pairs", []) | ||
| question_type = data.get("question_type", "") | ||
|
|
||
| if not isinstance(qa_pairs, list): | ||
| return jsonify({"error": "qa_pairs must be a list"}), 400 | ||
|
|
There was a problem hiding this comment.
There’s now new behavior to validate payload shape (qa_pairs must be a list) and return a 400. Since the repo has endpoint-level tests in backend/test_server.py, please add coverage for this branch (at least the 400 on non-list qa_pairs, which avoids any external Google API calls).
![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 (2)
eduaid_web/src/pages/Output.jsx (1)
129-149: Add deduping when merging MCQ arrays from both sources.This block appends
output_mcq.questionsandoutputdirectly; overlapping items can render twice and create unstable ordering. Add a deterministic dedupe key beforesetQaPairs.Example dedupe pass
- setQaPairs(combinedQaPairs); + const seen = new Set(); + const uniqueQaPairs = combinedQaPairs.filter((item) => { + const key = `${item.question_type}::${item.question}::${item.answer ?? ""}`; + if (seen.has(key)) return false; + seen.add(key); + return true; + }); + setQaPairs(uniqueQaPairs);Based on learnings: In Output.jsx, when multiple question types are generated,
output_mcq.questionsandoutputshould be coordinated 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 129 - 149, The merging of MCQ entries appends outputMcqQuestions and output into combinedQaPairs without deduplication, causing duplicate questions and unstable ordering; update the merge logic that pushes into combinedQaPairs (the block referencing outputMcqQuestions, qaPairsFromStorage["output_mcq"] / questionType === "get_mcq", and output) to perform a deterministic dedupe before calling setQaPairs: compute a stable dedupe key (e.g., normalized question text plus serialized options or question_statement) for each pushed object, track seen keys in a Set to skip duplicates, preserve the desired sort/order deterministically (e.g., push from storage first then generated, or sort by the dedupe key) and finally call setQaPairs with the filtered combined array.backend/server.py (1)
429-431: Harden link construction against missing Google API fields.Line 430 uses unsafe bracket access to
result['formId']which will throw aKeyErrorif the upstream API response lacks this field. The Google Forms API documentation confirms thatformIdandresponderUriare not guaranteed in all successful responses, particularlyresponderUrifor unpublished forms. Switch to safe.get()extraction and return a controlled error response for incomplete API data.Proposed hardening
- form_link = result.get("responderUri") - edit_link = f"https://docs.google.com/forms/d/{result['formId']}/edit" - return jsonify({"form_link": form_link, "edit_link": edit_link}) + form_link = result.get("responderUri") + form_id = result.get("formId") + if not form_id or not form_link: + return jsonify({"error": "Failed to create Google Form links"}), 502 + edit_link = f"https://docs.google.com/forms/d/{form_id}/edit" + return jsonify({"form_link": form_link, "edit_link": edit_link})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/server.py` around lines 429 - 431, The code currently assumes result['formId'] and responderUri exist and will KeyError; change direct bracket access to safe .get() calls (use form_id = result.get("formId") and responder_uri = result.get("responderUri")), validate that form_id (and responder_uri if required) is present, and if missing return a controlled JSON error via jsonify (e.g., jsonify({"error":"missing formId or responderUri"}) with appropriate status code) instead of constructing edit_link unconditionally; update where edit_link is built (edit_link = f"https://docs.google.com/forms/d/{form_id}/edit") and only return {"form_link": responder_uri, "edit_link": edit_link} when validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/server.py`:
- Around line 265-267: The handler currently assumes request.get_json() returns
an object and directly calls data.get(...), which fails for JSON arrays/strings;
validate that the parsed JSON (the variable data) is a dict before using .get.
Update the route handler around the request.get_json() call to check
isinstance(data, dict) (or equivalent) and return a clear 400/BadRequest when it
is not an object, then safely access qa_pairs and question_type only after that
check.
---
Nitpick comments:
In `@backend/server.py`:
- Around line 429-431: The code currently assumes result['formId'] and
responderUri exist and will KeyError; change direct bracket access to safe
.get() calls (use form_id = result.get("formId") and responder_uri =
result.get("responderUri")), validate that form_id (and responder_uri if
required) is present, and if missing return a controlled JSON error via jsonify
(e.g., jsonify({"error":"missing formId or responderUri"}) with appropriate
status code) instead of constructing edit_link unconditionally; update where
edit_link is built (edit_link =
f"https://docs.google.com/forms/d/{form_id}/edit") and only return {"form_link":
responder_uri, "edit_link": edit_link} when validated.
In `@eduaid_web/src/pages/Output.jsx`:
- Around line 129-149: The merging of MCQ entries appends outputMcqQuestions and
output into combinedQaPairs without deduplication, causing duplicate questions
and unstable ordering; update the merge logic that pushes into combinedQaPairs
(the block referencing outputMcqQuestions, qaPairsFromStorage["output_mcq"] /
questionType === "get_mcq", and output) to perform a deterministic dedupe before
calling setQaPairs: compute a stable dedupe key (e.g., normalized question text
plus serialized options or question_statement) for each pushed object, track
seen keys in a Set to skip duplicates, preserve the desired sort/order
deterministically (e.g., push from storage first then generated, or sort by the
dedupe key) and finally call setQaPairs with the filtered combined array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc7b7da6-3b2b-434e-b17c-40c3e5336e02
📒 Files selected for processing (4)
backend/server.pyeduaid_web/src/pages/Output.jsxextension/src/pages/question/Question.jsxextension/src/pages/question/SidePanel.jsx
| data = request.get_json() or {} | ||
| qa_pairs = data.get("qa_pairs", []) | ||
| question_type = data.get("question_type", "") |
There was a problem hiding this comment.
Validate JSON root type before calling .get().
At Line 265–266, a JSON array/string payload will make data.get(...) fail with 500 instead of returning a clear 400. Add an object-shape check first.
Proposed fix
- data = request.get_json() or {}
+ data = request.get_json(silent=True) or {}
+ if not isinstance(data, dict):
+ return jsonify({"error": "Request body must be a JSON object"}), 400
qa_pairs = data.get("qa_pairs", [])📝 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.
| data = request.get_json() or {} | |
| qa_pairs = data.get("qa_pairs", []) | |
| question_type = data.get("question_type", "") | |
| data = request.get_json(silent=True) or {} | |
| if not isinstance(data, dict): | |
| return jsonify({"error": "Request body must be a JSON object"}), 400 | |
| qa_pairs = data.get("qa_pairs", []) | |
| question_type = data.get("question_type", "") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/server.py` around lines 265 - 267, The handler currently assumes
request.get_json() returns an object and directly calls data.get(...), which
fails for JSON arrays/strings; validate that the parsed JSON (the variable data)
is a dict before using .get. Update the route handler around the
request.get_json() call to check isinstance(data, dict) (or equivalent) and
return a clear 400/BadRequest when it is not an object, then safely access
qa_pairs and question_type only after that check.
Addressed Issues:
Fixes #616
Screenshots/Recordings:
N/A (no visual UI redesign).
This PR is a reliability and API contract fix for Google Form generation flow across backend + clients.
Additional Notes:
/generate_gformresponse contract and make consumers backward-compatible.form_linkedit_linkform_link)400whenqa_pairsis not a list.AI Usage Disclosure:
I have used the following AI models and tools: GPT-5.3-Codex (GitHub Copilot) for drafting/refinement; all changes were reviewed and validated before submission.
Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes