Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Caution Review failedThe pull request is closed. WalkthroughCompleted-course state is moved to a JSON cookie ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Browser
participant ClientJS as Index (client JS)
participant Server as App Server
participant Unlocks as /unlocks handler
User->>Browser: GET /
Browser->>Server: GET /
Server-->>Browser: HTML (includes completed_courses)
Browser->>ClientJS: render + prepopulate from PRECOMPLETED
User->>ClientJS: submits form (AJAX)
ClientJS->>Server: POST /unlocks (body includes completed-courses JSON, view_type)
Server->>Unlocks: validate view_type, compute redirect URL
Unlocks-->>Server: create response via make_response, set `completed-courses` cookie, set Location header (redirect)
Server-->>ClientJS: 302 redirect response
ClientJS->>Browser: set window.location to redirect URL
Browser->>Server: GET redirected page
Server-->>Browser: Unlocks page (reads `completed-courses` cookie to compute prereqs)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
app/routes/main.py (3)
71-82: Harden the cookie (expiry, SameSite, Secure, HttpOnly).Set explicit attributes to reduce CSRF/snooping risk and ensure reasonable retention.
Apply this diff:
- resp.set_cookie("completed-courses", completed) + resp.set_cookie( + "completed-courses", + completed, + max_age=60 * 60 * 24 * 180, # 180 days + samesite="Lax", + secure=request.is_secure, + httponly=True, + path="/", + )
120-122: Remove stray triple-quoted string with typos.This is an inert string literal with spelling errors. Replace with a concise comment.
Apply this diff:
- """ - this readibility sucsk - """ + # TODO: Improve readability of the following loop.
123-141: Avoid duplicating prereq lookup logic; extract a shared helper.This re-implements the API’s prereq computation. Prefer extracting a utility (e.g., services/courses.py:get_direct_prereqs(graph, code, sem)) used by both the API and this view to keep behavior in sync and simplify maintenance.
I can draft a small helper module and wire both call-sites to it if you’d like.
app/templates/index.html (6)
90-92: Inline JSON directly; avoid JSON.parse roundtrip.Jinja’s tojson produces a JS literal. Using JSON.parse on it is unnecessary and adds failure surface.
Apply this diff:
- // Server-provided previously completed courses (from cookie) - const PRECOMPLETED = JSON.parse('{{ completed_courses | tojson | safe }}'); + // Server-provided previously completed courses (from cookie) + const PRECOMPLETED = {{ completed_courses | tojson }};
163-164: Anchor the validation regex to the end.Without $, inputs that merely start with a valid code will pass.
Apply this diff:
- const CODE_RE = /^[A-Z]{3,4}\d{4}[A-Z]?/; + const CODE_RE = /^[A-Z]{3,4}\d{4}[A-Z]?$/;
25-25: Rename temporary form id for clarity and update listener.“temp-form-name” reads as a placeholder. Use a stable, descriptive id.
Apply this diff:
- <form id="temp-form-name" action="{{ url_for('main.unlocks_redirect') }}" method="post"> + <form id="course-form" action="{{ url_for('main.unlocks_redirect') }}" method="post">- document.getElementById('temp-form-name').addEventListener('submit', function(e) { + document.getElementById('course-form').addEventListener('submit', function(e) {Also applies to: 165-165
201-213: Handle redirects explicitly to avoid fetching a full HTML response before navigation.Fetch follows the 302 and downloads the entire unlocks page, then you navigate to it again. Consider manual redirect to use the Location header directly.
Example change:
- fetch(this.action, { - method: 'POST', - body: formData - }).then(response => { - if (response.ok) { - window.location.href = response.url; - } else { - alert('Submission failed'); - } - }).catch(error => { + fetch(this.action, { + method: 'POST', + body: formData, + redirect: 'manual', + }).then(response => { + const loc = response.headers.get('Location'); + if (loc) { + window.location.assign(loc); + } else if (response.ok) { + window.location.assign(response.url); + } else { + alert('Submission failed'); + } + }).catch(error => { console.error('Error:', error); alert('Submission failed'); });
216-226: Remove duplicate input event listener.Two nearly identical listeners reset the border color; keep one.
Apply this diff:
- document.addEventListener('input', function(e) { - if (e.target.tagName === 'INPUT') { - e.target.style.borderColor = ''; - } - }); - document.addEventListener('input', function(e) { - if (e.target.tagName === 'INPUT') { - e.target.style.borderColor = ''; - } - }, true); + document.addEventListener('input', function(e) { + if (e.target.tagName === 'INPUT') { + e.target.style.borderColor = ''; + } + });
148-157: Clamp prepopulation target and call updateButtonStates after ensureInputCount.If PRECOMPLETED is very large, ensureInputCount will now cap additions, but it’s safer to clamp the requested count, then refresh controls immediately.
Apply this diff:
- if (Array.isArray(PRECOMPLETED) && PRECOMPLETED.length) { - ensureInputCount(Math.max(minCourses, PRECOMPLETED.length)); + if (Array.isArray(PRECOMPLETED) && PRECOMPLETED.length) { + ensureInputCount(Math.max(minCourses, PRECOMPLETED.length)); + updateButtonStates();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/routes/main.py(4 hunks)app/templates/index.html(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/routes/main.py (2)
app/config.py (1)
get_config(30-32)app/routes/api.py (2)
unlocks(9-20)prereqs(24-43)
🪛 Ruff (0.12.2)
app/routes/main.py
64-64: f-string without any placeholders
Remove extraneous f prefix
(F541)
🪛 GitHub Actions: Run Tests
app/routes/main.py
[error] 43-43: TypeError: the JSON object must be str, bytes or bytearray, not NoneType. 'completed' is None when json.loads(completed) is executed at app/routes/main.py:43 (triggered by tests/test_routes.py::test_home).
| code = request.form.get("tentative-code", "") | ||
| completed = request.form.get(f"completed-courses", "[]") | ||
| sem = request.form.get("semester", current_app.config["DEFAULT_SEMESTER"]) | ||
|
|
||
| view = request.form.get("view_type", "") | ||
| if view != "tcm" and view != "graph": | ||
| if view not in ("tcm", "graph"): | ||
| abort(400, "Bad view type") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary f-string and validate client-provided JSON before use.
- F541: f-string without placeholders at Line 64.
- Treat completed-courses as untrusted; parse, validate list type, normalise/dedupe, then re-serialize.
Apply this diff:
- code = request.form.get("tentative-code", "")
- completed = request.form.get(f"completed-courses", "[]")
+ code = request.form.get("tentative-code", "")
+ completed_raw = request.form.get("completed-courses", "[]")
+ try:
+ items = json.loads(completed_raw)
+ if not isinstance(items, list):
+ raise ValueError("completed-courses must be a list")
+ # Normalise, dedupe, and keep stable ordering
+ norm = {normalise(c) for c in items if isinstance(c, str) and normalise(c)}
+ completed = json.dumps(sorted(norm))
+ except (json.JSONDecodeError, TypeError, ValueError) as e:
+ current_app.logger.warning("Invalid completed-courses in form: %s", e)
+ completed = "[]"📝 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.
| code = request.form.get("tentative-code", "") | |
| completed = request.form.get(f"completed-courses", "[]") | |
| sem = request.form.get("semester", current_app.config["DEFAULT_SEMESTER"]) | |
| view = request.form.get("view_type", "") | |
| if view != "tcm" and view != "graph": | |
| if view not in ("tcm", "graph"): | |
| abort(400, "Bad view type") | |
| code = request.form.get("tentative-code", "") | |
| completed_raw = request.form.get("completed-courses", "[]") | |
| try: | |
| items = json.loads(completed_raw) | |
| if not isinstance(items, list): | |
| raise ValueError("completed-courses must be a list") | |
| # Normalise, dedupe, and keep stable ordering | |
| norm = {normalise(c) for c in items if isinstance(c, str) and normalise(c)} | |
| completed = json.dumps(sorted(norm)) | |
| except (json.JSONDecodeError, TypeError, ValueError) as e: | |
| current_app.logger.warning("Invalid completed-courses in form: %s", e) | |
| completed = "[]" | |
| sem = request.form.get("semester", current_app.config["DEFAULT_SEMESTER"]) | |
| view = request.form.get("view_type", "") | |
| if view not in ("tcm", "graph"): | |
| abort(400, "Bad view type") |
🧰 Tools
🪛 Ruff (0.12.2)
64-64: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
In app/routes/main.py around lines 63 to 69, replace the unnecessary f-string on
line 64 with a plain string key and validate the client-provided
"completed-courses" JSON before use: read the raw value via
request.form.get("completed-courses", "[]"), parse it with json.loads inside a
try/except and abort(400) on JSONDecodeError, ensure the parsed value is a list
(abort 400 if not), normalize entries (e.g., coerce to str, strip whitespace),
remove duplicates while preserving order, and then either use the validated list
directly or re-serialize with json.dumps when a string is required downstream;
also ensure any subsequent code expects the sanitized list instead of the raw
untrusted string.
| // Helper to ensure at least n inputs exist (adding in step-sized batches without exceeding max) | ||
| function ensureInputCount(n) { | ||
| let currentCount = courseInputsDiv.querySelectorAll('input').length; | ||
| while (currentCount < n) { | ||
| for (let i = 0; i < 5; i++) { | ||
| courseInputsDiv.appendChild(createCourseInput(currentCount + 1)); | ||
| currentCount++; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ensureInputCount ignores step/max and can over-append inputs.
Respect step and clamp to maxCourses to prevent runaway DOM growth when PRECOMPLETED is large.
Apply this diff:
- // Helper to ensure at least n inputs exist (adding in step-sized batches without exceeding max)
- function ensureInputCount(n) {
- let currentCount = courseInputsDiv.querySelectorAll('input').length;
- while (currentCount < n) {
- for (let i = 0; i < 5; i++) {
- courseInputsDiv.appendChild(createCourseInput(currentCount + 1));
- currentCount++;
- }
- }
- }
+ // Helper to ensure at least n inputs exist (respecting step and max)
+ function ensureInputCount(n) {
+ const target = Math.min(n, maxCourses);
+ let currentCount = courseInputsDiv.querySelectorAll('input').length;
+ while (currentCount < target) {
+ const toAdd = Math.min(step, target - currentCount);
+ for (let i = 0; i < toAdd; i++) {
+ courseInputsDiv.appendChild(createCourseInput(currentCount + 1));
+ currentCount++;
+ }
+ }
+ }📝 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.
| // Helper to ensure at least n inputs exist (adding in step-sized batches without exceeding max) | |
| function ensureInputCount(n) { | |
| let currentCount = courseInputsDiv.querySelectorAll('input').length; | |
| while (currentCount < n) { | |
| for (let i = 0; i < 5; i++) { | |
| courseInputsDiv.appendChild(createCourseInput(currentCount + 1)); | |
| currentCount++; | |
| } | |
| } | |
| } | |
| // Helper to ensure at least n inputs exist (respecting step and max) | |
| function ensureInputCount(n) { | |
| const target = Math.min(n, maxCourses); | |
| let currentCount = courseInputsDiv.querySelectorAll('input').length; | |
| while (currentCount < target) { | |
| const toAdd = Math.min(step, target - currentCount); | |
| for (let i = 0; i < toAdd; i++) { | |
| courseInputsDiv.appendChild(createCourseInput(currentCount + 1)); | |
| currentCount++; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/templates/index.html around lines 137 to 146, ensureInputCount currently
ignores the configured step and maxCourses and can append too many inputs;
change the loop to respect step and clamp to maxCourses by computing a batchSize
= Math.min(step, n - currentCount, maxCourses - currentCount) on each iteration,
append only batchSize inputs (incrementing currentCount accordingly), and break
the loop when batchSize <= 0 to avoid runaway DOM growth when PRECOMPLETED or n
is large.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/routes/main.py (2)
63-65: Remove unnecessary f-string (Ruff F541) and validate client-provided JSON before setting the cookie.Sanitize, normalize, and re-serialize to a canonical JSON array string so downstream routes consume trusted input.
Apply this diff:
- code = request.form.get("tentative-code", "") - completed = request.form.get(f"completed-courses", "[]") + code = request.form.get("tentative-code", "") + completed_raw = request.form.get("completed-courses", "[]") + try: + items = json.loads(completed_raw) + if not isinstance(items, list): + raise ValueError("completed-courses must be a list") + norm_set: set[str] = set() + for c in items: + if isinstance(c, str): + n = normalise(c) + if n: + norm_set.add(n) + completed = json.dumps(sorted(norm_set)) + except (json.JSONDecodeError, TypeError, ValueError) as e: + current_app.logger.warning("Invalid completed-courses in form: %s", e) + completed = "[]"
91-97: Mirror robust cookie parsing here (default to "[]", catch TypeError, and normalize).This route still crashes on json.loads(None) and set(...) on scalars; it also needs normalization for reliable comparisons.
Apply this diff:
- completed = request.cookies.get("completed-courses") - try: - completed = set(json.loads(completed)) - except json.JSONDecodeError as e: - current_app.logger.exception(f"Error decoding JSON: {e}") - completed = set() + completed_raw = request.cookies.get("completed-courses") + try: + items = json.loads(completed_raw or "[]") + if not isinstance(items, list): + raise ValueError("completed-courses cookie must be a JSON list") + completed = set() + for c in items: + if isinstance(c, str): + n = normalise(c) + if n: + completed.add(n) + except (json.JSONDecodeError, TypeError, ValueError) as e: + current_app.logger.exception("Error decoding completed-courses cookie: %s", e) + completed = set()
🧹 Nitpick comments (3)
app/routes/main.py (3)
71-82: Set cookie security attributes and a finite lifetime.Prefer HttpOnly + SameSite=Lax and Secure from config. This reduces XSS exposure and CSRF surface; also bound cookie size/lifetime.
Apply this diff:
- resp = make_response( + resp = make_response( redirect( url_for( "main.unlocks_page", code=code, semester=sem, view_type=view, ) ) ) - resp.set_cookie("completed-courses", completed) + resp.set_cookie( + "completed-courses", + completed, + max_age=60 * 60 * 24 * 180, # 180 days + httponly=True, + samesite="Lax", + secure=bool(current_app.config.get("SESSION_COOKIE_SECURE", False)), + ) return respOptional: enforce a conservative size cap before setting the cookie (e.g., if len(completed) > 3500: log and fall back to "[]").
120-122: Remove stray triple-quoted strings.These are no-op runtime string literals that reduce readability and slightly bloat bytecode. Replace with comments or delete.
Apply this diff:
- """ - this readibility sucsk - """ + # (removed stray string literal) @@ - """ - Get prerequisites for course c using the same logic as the url_for(api_bp.prereqs()) API route - please review this code carefully in code review. It may be wise to opt for an internal API call if we want - to use the prereqs API elsewhere in the app. For now, it is more efficient to just duplicate the logic - """ + # Get prerequisites for course c using the same logic as the API route. + # Consider refactoring to a shared helper if the logic is reused elsewhere.Also applies to: 124-128
41-47: Optional: factor cookie parsing into a helper to avoid duplication.Both routes now share identical parsing/normalization. Extract into a private helper for maintainability.
Example helper to add near normalise():
def _parse_completed_cookie(raw: str | None) -> set[str]: try: items = json.loads(raw or "[]") if not isinstance(items, list): raise ValueError("completed-courses must be a list") out: set[str] = set() for c in items: if isinstance(c, str): n = normalise(c) if n: out.add(n) return out except (json.JSONDecodeError, TypeError, ValueError): current_app.logger.exception("Error decoding completed-courses cookie") return set()Then use
completed = _parse_completed_cookie(request.cookies.get("completed-courses"))in both places. I can push this refactor if you want.Also applies to: 91-97
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/routes/main.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/routes/main.py (2)
app/config.py (1)
get_config(30-32)app/routes/api.py (2)
unlocks(9-20)prereqs(24-43)
🪛 Ruff (0.12.2)
app/routes/main.py
64-64: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (3)
app/routes/main.py (3)
13-16: Imports look good.Adding make_response and json is appropriate for the new cookie-based flow.
68-69: LGTM: view_type validation.Early gating to allowed values is correct.
110-113: Error message may misattribute ValueError source.struct.postreqs can raise ValueError for reasons other than “course not found” (e.g., bad semester). Consider surfacing the actual message or distinguishing causes.
If you want to keep it minimal:
- except ValueError: - abort(400, f"Course not found in catalog: {base}") + except ValueError as e: + abort(400, str(e))
| completed = request.cookies.get("completed-courses", "[]") | ||
| try: | ||
| completed = set(json.loads(completed)) | ||
| except json.JSONDecodeError as e: | ||
| current_app.logger.exception(f"Error decoding JSON: {e}") | ||
| completed = set() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden cookie parsing and normalize course codes to prevent crashes and inconsistent matches.
json.loads may return non-lists (e.g., "5"), making set(...) raise TypeError; elements may also be unnormalized, breaking set logic against graph codes. Validate type, normalize via normalise(), dedupe, and log with parameterized logging.
Apply this diff:
- completed = request.cookies.get("completed-courses", "[]")
- try:
- completed = set(json.loads(completed))
- except json.JSONDecodeError as e:
- current_app.logger.exception(f"Error decoding JSON: {e}")
- completed = set()
-
- # Convert to a sorted list for stable JSON serialization in template (sets are not JSON serializable)
- completed_list = sorted(completed)
+ completed_raw = request.cookies.get("completed-courses")
+ try:
+ items = json.loads(completed_raw or "[]")
+ if not isinstance(items, list):
+ raise ValueError("completed-courses cookie must be a JSON list")
+ norm: set[str] = set()
+ for c in items:
+ if isinstance(c, str):
+ n = normalise(c)
+ if n:
+ norm.add(n)
+ completed = norm
+ except (json.JSONDecodeError, TypeError, ValueError) as e:
+ current_app.logger.exception("Error decoding completed-courses cookie: %s", e)
+ completed = set()
+
+ # Convert to a sorted list for stable JSON serialization in template
+ completed_list = sorted(completed)Also applies to: 48-50, 57-57
🤖 Prompt for AI Agents
In app/routes/main.py around lines 41-47 (also apply same changes at 48-50 and
57), json.loads may return non-list or contain unnormalized items causing
TypeError and inconsistent matches; after loading, verify the result is a list,
coerce/ignore non-string items, run each string through normalise(), build a
deduplicated set from the normalized values, and replace direct
set(json.loads(...)) with this validated flow; on JSON decode or unexpected
types log errors using parameterized logging (no f-strings) and ensure fallback
to an empty set when invalid.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/routes/main.py (3)
41-49: Harden cookie parsing: validate type, normalize course codes, and avoid broadexcept.Current code trusts cookie JSON and builds a set directly. If the cookie holds a non-list (e.g., "5"),
set(...)raisesTypeError; also, codes aren’t normalized, so comparisons against graph codes may fail. Prefer narrow exceptions and parameterized logging. This mirrors prior feedback.Apply this diff:
- completed = request.cookies.get("completed-courses", "[]") - try: - completed = set(json.loads(completed)) - except Exception as e: - current_app.logger.exception(f"Error decoding JSON: {e}") - completed = set() - - # Convert to a sorted list for stable JSON serialization in template (sets are not JSON serializable) - completed_list = sorted(completed) + completed_raw = request.cookies.get("completed-courses") + try: + items = json.loads(completed_raw or "[]") + if not isinstance(items, list): + raise ValueError("completed-courses cookie must be a JSON list") + norm: set[str] = set() + for c in items: + if isinstance(c, str): + n = normalise(c) + if n: + norm.add(n) + completed = norm + except (json.JSONDecodeError, TypeError, ValueError) as e: + current_app.logger.exception("Error decoding completed-courses cookie: %s", e) + completed = set() + + # Convert to a sorted list for stable JSON serialization in template + completed_list = sorted(completed)
63-69: Sanitize client JSON and set a secure, long‑lived cookie; drop unnecessary f-string; return 303.
- Remove the f-string on Line 64 (Ruff F541).
- Parse/validate/normalize/dedupe the client-provided JSON, and enforce MAX_COURSES_TAKEN.
- Set cookie with security attributes (HttpOnly, SameSite, Secure) and a reasonable TTL.
- Use a 303 redirect for POST/Redirect/GET.
Apply this diff:
- code = request.form.get("tentative-code", "") - completed = request.form.get(f"completed-courses", "[]") + code = request.form.get("tentative-code", "") + completed_raw = request.form.get("completed-courses", "[]") + try: + items = json.loads(completed_raw) + if not isinstance(items, list): + raise ValueError("completed-courses must be a list") + # normalize, dedupe, and cap to configured max (preserve order) + max_n = int(getattr(config, "MAX_COURSES_TAKEN", 20)) + norm: list[str] = [] + seen: set[str] = set() + for c in items: + if isinstance(c, str): + n = normalise(c) + if n and n not in seen: + seen.add(n) + norm.append(n) + if len(norm) >= max_n: + break + completed_json = json.dumps(norm) + except (json.JSONDecodeError, TypeError, ValueError) as e: + current_app.logger.warning("Invalid completed-courses in form: %s", e) + completed_json = "[]" @@ - resp = make_response( - redirect( + resp = make_response( + redirect( url_for( "main.unlocks_page", code=code, semester=sem, view_type=view, - ) + ), + code=303, ) ) - resp.set_cookie("completed-courses", completed) + resp.set_cookie( + "completed-courses", + completed_json, + max_age=60 * 60 * 24 * 180, # ~180 days + httponly=True, + samesite="Lax", + secure=request.is_secure, + path="/", + ) return respAlso applies to: 71-83
91-96: Fix crash when cookie is absent or malformed; normalize cookie values.
json.loads(None)raisesTypeError, and a non-list JSON causes downstream errors. Mirror the index-route parsing and normalize codes for consistent set membership checks. This was flagged previously; ensure it’s actually applied here.Apply this diff:
- completed = request.cookies.get("completed-courses") - try: - completed = set(json.loads(completed)) - except json.JSONDecodeError as e: - current_app.logger.exception(f"Error decoding JSON: {e}") - completed = set() + completed_raw = request.cookies.get("completed-courses") + try: + items = json.loads(completed_raw or "[]") + if not isinstance(items, list): + raise ValueError("completed-courses cookie must be a JSON list") + completed = {normalise(c) for c in items if isinstance(c, str) and normalise(c)} + except (json.JSONDecodeError, TypeError, ValueError) as e: + current_app.logger.exception("Error decoding completed-courses cookie: %s", e) + completed = set()
🧹 Nitpick comments (2)
app/routes/main.py (2)
120-122: Remove stray triple-quoted string; replace with a comment.It’s not a docstring and adds noise (typos included).
Apply this diff:
- """ - this readibility sucsk - """ + # TODO: Improve readability of the unlocks logic
13-16: Minor: split combined import and keep imports one-per-line (PEP 8 E401).Not blocking, just future-proofing for linters.
Suggested tweak:
-from flask import ( +from flask import ( @@ -) -from app.config import get_config -import re, json +) +from app.config import get_config +import json +import re
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/routes/main.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/routes/main.py (3)
tests/conftest.py (1)
app(7-14)app/config.py (1)
get_config(30-32)app/routes/api.py (2)
unlocks(9-20)prereqs(24-43)
🪛 Ruff (0.12.2)
app/routes/main.py
64-64: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (1)
app/routes/main.py (1)
68-69: LGTM: view validation is clear and strict.Membership check against a closed set keeps the route contract tight.
| completed = request.cookies.get("completed-courses", "[]") | ||
| try: | ||
| completed = set(json.loads(completed)) | ||
| except Exception as e: | ||
| current_app.logger.exception(f"Error decoding JSON: {e}") | ||
| completed = set() | ||
|
|
||
| # Convert to a sorted list for stable JSON serialization in template (sets are not JSON serializable) | ||
| completed_list = sorted(completed) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add tests for cookie handling edge cases.
Cover: no cookie, invalid JSON (e.g., "5"), non-list JSON (e.g., "{}"), mixed/invalid items, lowercase and spaced codes → all should not crash and should normalize consistently.
Run this quick check to find all cookie reads that must tolerate None/invalid JSON:
Also applies to: 91-96
🏁 Script executed:
#!/bin/bash
# Verify all cookie reads of "completed-courses" guard against None/invalid JSON
rg -n -C2 'cookies\.get\(\s*"completed-courses"[^)]*\)' app/routes/main.py
rg -n -C2 'json\.loads\(' app/routes/main.pyLength of output: 928
Unify and Harden Cookie Handling; Add Edge-Case Tests
The two completed-courses cookie reads in app/routes/main.py are handled inconsistently and may still crash on missing or malformed input. We should:
- Ensure both reads supply a safe default (
"[]") to avoidNone→json.loads(None)errors - Broaden the exception handler to catch both
json.JSONDecodeErrorandTypeError(the latter arises whencompletedisNone) - Add comprehensive tests covering:
- No cookie present
- Invalid JSON (e.g.
"5") - Non-list JSON (e.g.
"{}") - Mixed or invalid items within the array
- Course codes in lowercase or with extra whitespace (should normalize to canonical form)
Locations needing updates:
- Lines 41–49: already uses
cookies.get("completed-courses", "[]"), but confirm exception handling covers all error types. - Lines 91–96: change to
request.cookies.get("completed-courses", "[]")and extendexceptto catchTypeErroras well.
Suggested shell check to find all occurrences:
#!/bin/bash
rg -n -C2 'cookies\.get\(\s*"completed-courses"' app/routes/main.py
rg -n -C2 'json\.loads\(' app/routes/main.pyLet me know if you’d like help writing those tests or hardening the exception logic.
🤖 Prompt for AI Agents
In app/routes/main.py around lines 41–49 and 91–96, unify and harden the
handling of the "completed-courses" cookie by always using
request.cookies.get("completed-courses", "[]") as the input to json.loads,
validate that the decoded value is a list (otherwise treat as empty), and
broaden the exception handling to catch json.JSONDecodeError and TypeError;
after loading, normalize items by trimming whitespace and uppercasing (and only
keep string items), convert to a set for deduplication and fall back to an empty
set on any error, then produce a stable sorted list for templates; also add
tests for no cookie, invalid JSON (e.g. "5"), non-list JSON (e.g. "{}"),
mixed/invalid array items, and inputs with lowercase/whitespace to assert
normalization to canonical course codes.
|
@coderabbitai ignore |
✅ Actions performedReviews paused. |
1 participant
Completed courses are now stored in a cookie so the user doesn't have to input them over and over again.
Summary by CodeRabbit
New Features
Improvements