Course progression — durable writes, attempt counter, anon-to-user migration, creator dashboard#2
Open
tusharbisht wants to merge 2 commits into
Open
Course progression — durable writes, attempt counter, anon-to-user migration, creator dashboard#2tusharbisht wants to merge 2 commits into
tusharbisht wants to merge 2 commits into
Conversation
…n→user migration, creator dashboard
Audited progress tracking across the three interfaces (Web / Terminal CLI /
VS Code) against the backend (UserProgress / Enrollment / Certificate) and
landed six fixes that were blocking correctness or creator visibility.
Backend:
- _normalize_score(raw) — single helper that maps any inbound score (raw
0-1 from Web, int 0-100 from CLI/VSCode, "85", null) to the canonical
0.0-1.0 scale before write. Stops UserProgress.score from accumulating
mixed scales that broke every downstream avg / certificate / dashboard
number. Wired into POST /api/progress/complete.
- UserProgress.attempts (int, default 0) — new column added via
_ensure_column. POST /api/exercises/validate now calls _record_attempt
on every grader invocation, server-side counter for the creator
dashboard's per-step pass-rate.
- Anonymous identity (sll_anon cookie, 1y httpOnly). Issued by the
session_middleware on first request when no auth session exists; stored
on request.state.anon_id. Replaces the shared user_id="default" bucket
for guests so progress doesn't bleed across unrelated sessions.
- _migrate_anon_progress(anon_id, user_id) — runs at /api/auth/register
AND /api/auth/login. Transfers UserProgress + Certificate rows from the
anon bucket to the authenticated user_id; merges on conflict (max
score, sum attempts, OR completed); deletes anon rows after merge so
re-runs are idempotent. Anon cookie cleared on the response.
- GET /api/auth/creator/courses/{id}/aggregate-stats — new creator
endpoint. Returns enrollment summary, course-wide avg/median progress,
last-active distribution (24h/week/month/older/never), per-module
funnel (reached vs completed learner counts), per-step rollup
(attempts, completed, avg_score, pass_rate). Auth-scoped to the
course's creator. Depends on attempts (#4) and normalized score (#3)
to be meaningful.
Frontend:
- Web's POST /api/progress/complete is now awaited (was fire-and-forget)
with a localStorage retry queue (PROGRESS_PENDING_KEY). Failed writes
queue and drain on next page load via _drainPendingProgressWrites
called from init(). Closes the tab-close-mid-flight loss window.
- Web fetches GET /api/progress/{course_id} on enter-course and merges
backend completion state into localStorage before render. Sign-in on
a fresh device now sees the user's actual progress, not an empty cache.
- Creator dashboard: loadCreatorLearnersForCourse fetches aggregate-stats
in parallel with the existing enrolled-learners roster. New
renderCreatorAggregateOverview renders 4 metric cards (enrolled /
completed / avg / median), a coloured last-active distribution bar,
and a per-module per-step pass-rate heatmap (red <40% / amber <70% /
green ≥70%) above the existing learners table.
End-to-end smoke (in-process against fresh sqlite):
- _record_attempt × 3 → attempts column reaches 3 ✓
- _normalize_score(85) → 0.85 ✓
- _migrate_anon_progress: anon row transferred + merged into user_id with
attempts/score/completed preserved; anon row deleted ✓
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
the
size:XL
…/staging Followup to course-progression #1-#6: UserProgress had zero indices, so every progress read/write was a sequential scan. The earlier PR amplified this — _record_attempt now fires on every /exercises/validate, and /aggregate-stats does N+M+1 queries per dashboard load. Felt on Postgres real data (the user flagged: dev + staging are PG, not SQLite). Two indices added: - ix_user_progress_user_step (user_id, step_id) — covers the upsert hot path (mark_step_complete + _record_attempt) and any user_id-scoped reads (get_progress, enrolled-learners). - ix_user_progress_step_id (step_id) — covers the reverse: aggregate-stats per-step rollup that filters WHERE step_id IN (...). Defined on the model via __table_args__ (so fresh tables get them at create_all time) AND via idempotent CREATE INDEX IF NOT EXISTS in create_tables() (so existing dev/staging DBs get backfilled on next service restart). New _run_ddl helper for the index DDL — _ensure_column wasn't the right shape since CREATE INDEX IF NOT EXISTS is its own idempotency mechanism. CREATE INDEX IF NOT EXISTS works on both SQLite 3.8.0+ and Postgres 9.5+. Smoke (in-process): - Fresh DB → create_tables creates both indices via metadata.create_all - Re-run create_tables → no change, idempotent - Pre-migration sim (table without indices) → post-migration sim creates both via the IF NOT EXISTS DDL Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
CodeAnt AI finished reviewing your PR. |
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XL
Sequence DiagramThis diagram shows how guest progress is tied to an anonymous cookie, migrated to the authenticated user on register or login, and then surfaced as aggregate engagement and pass rate signals on the creator dashboard. sequenceDiagram
participant Learner
participant WebUI
participant Backend
participant Creator
Learner->>WebUI: Complete course steps as guest
WebUI->>Backend: Record progress tagged with anon cookie id
Learner->>Backend: Register or log in
Backend->>Backend: Migrate anon progress to user id and clear anon cookie
Backend-->>Learner: Account confirmed with preserved progress
Creator->>WebUI: Open creator dashboard for course
WebUI->>Backend: Request learners and aggregate course stats
Backend-->>WebUI: Return enrollment, progress, and per step pass rates
WebUI-->>Creator: Render overview metrics and step heatmap
Generated by CodeAnt AI |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+742
to
+777
| select( | ||
| _UP.step_id, | ||
| _func.coalesce(_func.sum(_UP.attempts), 0).label("attempts"), | ||
| _func.sum(case((_UP.completed.is_(True), 1), else_=0)).label("completed"), | ||
| _func.count(distinct(_UP.user_id)).label("learners"), | ||
| _func.avg(_UP.score).label("avg_score"), | ||
| ) | ||
| .where(_UP.step_id.in_(all_step_ids)) | ||
| .group_by(_UP.step_id) | ||
| )).all() | ||
| for r in rows: | ||
| sid = r.step_id | ||
| if sid in step_stats: | ||
| step_stats[sid] = { | ||
| "attempts": int(r.attempts or 0), | ||
| "completed": int(r.completed or 0), | ||
| "learners": int(r.learners or 0), | ||
| "avg_score": float(r.avg_score) if r.avg_score is not None else None, | ||
| } | ||
|
|
||
| # Per-module funnel: distinct learners who have ≥1 progress row on any | ||
| # step of the module (regardless of completion). | ||
| per_module = [] | ||
| for m in modules: | ||
| sids = module_step_ids.get(m.id, []) | ||
| # Reach: distinct user_ids with any progress in this module | ||
| reach_q = select(_func.count(distinct(_UP.user_id))).where(_UP.step_id.in_(sids)) if sids else None | ||
| reached = int((await db.execute(reach_q)).scalar() or 0) if reach_q is not None else 0 | ||
| # Module completion: distinct user_ids who completed ALL steps in the module | ||
| if sids: | ||
| sub_q = ( | ||
| select(_UP.user_id) | ||
| .where(_UP.step_id.in_(sids), _UP.completed.is_(True)) | ||
| .group_by(_UP.user_id) | ||
| .having(_func.count(_UP.step_id) >= len(sids)) | ||
| ) |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
The creator aggregate stats endpoint computes step/module metrics from all UserProgress rows for the course's step IDs, while the dashboard denominator (enrolled count) comes from Enrollment only. Anonymous and non-enrolled users' progress is included in reach/completion counts but excluded from enrollments, so per-module funnel percentages can exceed 100% of enrolled and misrepresent engagement.
Suggestion: Restrict aggregate queries (per-step and per-module rollups) to user_ids that have an Enrollment row for the target course, and explicitly exclude anonymous/default buckets from creator analytics so all percentages use a consistent enrolled-learner population.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** backend/auth.py
**Line:** 742:777
**Comment:**
*HIGH: The creator aggregate stats endpoint computes step/module metrics from all UserProgress rows for the course's step IDs, while the dashboard denominator (enrolled count) comes from Enrollment only. Anonymous and non-enrolled users' progress is included in reach/completion counts but excluded from enrollments, so per-module funnel percentages can exceed 100% of enrolled and misrepresent engagement.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+242
to
+247
| response = await call_next(request) | ||
| if getattr(request.state, "_issue_anon_cookie", False): | ||
| # Mint the cookie on the way out so it's bound to the response | ||
| # the browser is about to receive. | ||
| _set_anon_cookie(response, anon_id) | ||
| return response |
There was a problem hiding this comment.
Suggestion: The middleware always sets sll_anon after call_next when the request arrived without that cookie, so endpoint-level deletes are overridden on the way out. On first-time /register or /login requests this re-creates the anon cookie immediately after _clear_anon_cookie(response), breaking the intended cleanup behavior. Skip re-issuing when the response already deleted the cookie (or clear the middleware flag before returning from auth endpoints). [logic error]
Severity Level: Major ⚠️
- ⚠️ Anonymous IDs persist after signup despite explicit cleanup.
- ⚠️ Pre-signup anon IDs linger on newly registered sessions.
- ⚠️ Future migration logic must handle unnecessary anon cookies.Steps of Reproduction ✅
1. Start the FastAPI app with `session_middleware` registered as HTTP middleware as
documented in `backend/auth.py:20-25`
(`app.middleware("http")(_auth.session_middleware)`).
2. Make a POST request to the registration endpoint `/api/auth/register` (handler
`register()` in `backend/auth.py:355-386`) from a fresh browser context that has no
`sll_anon` cookie (e.g., new incognito window).
3. In `session_middleware` (`backend/auth.py:188-247`), the request has no `sll_anon`
cookie, so lines 234-240 generate a new `anon_id = uuid.uuid4().hex` and set
`request.state._issue_anon_cookie = True` before calling `response = await
call_next(request)` at line 242.
4. Inside `register()`, the handler reads `request.state.anon_id` for migration
(`backend/auth.py:371-377`), then calls `_clear_anon_cookie(response)` at line 384 to
delete the anon cookie so "subsequent requests use the session cookie"; after the handler
returns, `session_middleware` resumes at lines 243-246, sees `_issue_anon_cookie` still
`True`, and calls `_set_anon_cookie(response, anon_id)`, re‑adding the anon cookie in the
same response that attempted to clear it, so the browser ends up with a fresh `sll_anon`
even though the endpoint tried to delete it.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/auth.py
**Line:** 242:247
**Comment:**
*Logic Error: The middleware always sets `sll_anon` after `call_next` when the request arrived without that cookie, so endpoint-level deletes are overridden on the way out. On first-time `/register` or `/login` requests this re-creates the anon cookie immediately after `_clear_anon_cookie(response)`, breaking the intended cleanup behavior. Skip re-issuing when the response already deleted the cookie (or clear the middleware flag before returning from auth endpoints).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+741
to
+751
| rows = (await db.execute( | ||
| select( | ||
| _UP.step_id, | ||
| _func.coalesce(_func.sum(_UP.attempts), 0).label("attempts"), | ||
| _func.sum(case((_UP.completed.is_(True), 1), else_=0)).label("completed"), | ||
| _func.count(distinct(_UP.user_id)).label("learners"), | ||
| _func.avg(_UP.score).label("avg_score"), | ||
| ) | ||
| .where(_UP.step_id.in_(all_step_ids)) | ||
| .group_by(_UP.step_id) | ||
| )).all() |
There was a problem hiding this comment.
Suggestion: The new aggregate queries are not scoped to enrolled learners, so any anonymous or non-enrolled user activity is included in creator analytics. This can make reached_learners, completed_learners, and step learner counts exceed course enrollment and produces misleading dashboard funnels. Join/filter these stats by Enrollment.course_id == course_id and enrolled user IDs. [logic error]
Severity Level: Critical 🚨
- ❌ Creator dashboard funnels overcount non-enrolled anonymous learners.
- ❌ Per-step pass rates mix guest and enrolled learner behavior.
- ⚠️ Course optimization decisions may be based on skewed analytics.Steps of Reproduction ✅
1. As an anonymous learner (no auth cookie), submit progress for a course by calling
`/api/progress/complete` (handler `mark_step_complete()` in `backend/main.py:99-123`);
when `auth_user` is `None`, lines 118-123 set `user_id = request.state.anon_id or
"default"` and write `UserProgress` rows for that `user_id` without creating any
`Enrollment` record.
2. Do not enroll this anon learner in the course (no call to
`/api/auth/enroll/{course_id}`, which is the only place that creates `Enrollment` rows in
`backend/auth.py:496-517`), so the course's `enrollments` set still only contains
authenticated users.
3. As the course creator, call the creator analytics endpoint
`/api/auth/creator/courses/{course_id}/aggregate-stats` implemented by
`creator_course_aggregate_stats()` in `backend/auth.py:692-853`; lines 741-751 build
per-step stats by querying `UserProgress` (`_UP`) with
`.where(_UP.step_id.in_(all_step_ids))` and no filter on `Enrollment` or on `user_id`
membership in the enrolled cohort, so the anon learner's rows are included in `attempts`,
`completed`, and `learners` counts.
4. In the same response, lines 809-849 compute `enrollments` only from `Enrollment` rows
filtered by `Enrollment.course_id == course_id` and derive `"summary.enrolled"` and
progress distributions from those authenticated enrollments; as a result, module
`reached_learners`, `completed_learners`, and per-step `learners` can exceed the enrolled
count and include anonymous or otherwise non-enrolled activity, producing misleading
funnel and pass-rate statistics on the creator dashboard.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/auth.py
**Line:** 741:751
**Comment:**
*Logic Error: The new aggregate queries are not scoped to enrolled learners, so any anonymous or non-enrolled user activity is included in creator analytics. This can make `reached_learners`, `completed_learners`, and step learner counts exceed course enrollment and produces misleading dashboard funnels. Join/filter these stats by `Enrollment.course_id == course_id` and enrolled user IDs.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+835
to
+837
| median_pct = ( | ||
| sorted(progress_percents)[len(progress_percents) // 2] | ||
| if progress_percents else 0 |
There was a problem hiding this comment.
Suggestion: The median calculation is incorrect for even-sized enrollment sets because it picks only one middle element instead of averaging the two center values. This biases reported median progress upward or downward depending on ordering. Compute the true median for even counts. [logic error]
Severity Level: Major ⚠️
- ⚠️ Median course progress reported to creators is mathematically wrong.
- ⚠️ Creator dashboard summaries misrepresent learner cohort central tendency.Steps of Reproduction ✅
1. Have at least two authenticated learners enrolled in the same course via
`/api/auth/enroll/{course_id}` (`backend/auth.py:496-517`), and let them complete
different fractions of the course so `Enrollment.progress_percent` values diverge (kept in
sync from `/api/progress/complete` in `backend/main.py:45-63` and `2100-63`).
2. For a concrete example, let one learner reach 40% and another 80% progress on the
course; this results in two `Enrollment` rows for that course with `progress_percent` 40
and 80 in the database (`backend/database.py:347-363`).
3. As the creator, call `/api/auth/creator/courses/{course_id}/aggregate-stats`
(`creator_course_aggregate_stats()` in `backend/auth.py:692-853`); lines 809-820 build
`progress_percents` from the enrolled learners' `progress_percent` fields, so in this
example the list is `[40, 80]`.
4. The median is computed at lines 835-837 as
`sorted(progress_percents)[len(progress_percents) // 2]`, which for two elements picks
index `1` (value `80`) instead of the statistical median `(40 + 80) / 2 = 60`, so for any
even number of enrollments the reported `median_progress_percent` in the JSON summary is
systematically incorrect.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/auth.py
**Line:** 835:837
**Comment:**
*Logic Error: The median calculation is incorrect for even-sized enrollment sets because it picks only one middle element instead of averaging the two center values. This biases reported median progress upward or downward depending on ordering. Compute the true median for even counts.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+230
to
+233
| __table_args__ = ( | ||
| Index("ix_user_progress_user_step", "user_id", "step_id"), | ||
| Index("ix_user_progress_step_id", "step_id"), | ||
| ) |
There was a problem hiding this comment.
Suggestion: The (user_id, step_id) index is non-unique, but write paths use read-then-insert upsert logic for progress/attempts. Under concurrent requests, duplicate rows for the same user-step can be inserted, which inflates attempts/completions and breaks migration/aggregation assumptions. Enforce uniqueness with a unique constraint/index on (user_id, step_id). [race condition]
Severity Level: Critical 🚨
- ❌ Concurrent writes can create duplicate user-step progress rows.
- ⚠️ Progress reads pick arbitrary row for a given step.
- ⚠️ Attempt counters and completion checks become inconsistent under load.Steps of Reproduction ✅
1. Note that `UserProgress` in `backend/database.py:203-233` models logical keys as
`(user_id, step_id)` but only defines non-unique indices in `__table_args__` at lines
230-233 (plain `Index` on `("user_id", "step_id")` and on `("step_id")`, no uniqueness or
constraint).
2. The write paths both implement manual upserts by doing a SELECT-then-INSERT:
`/api/progress/complete` (`mark_step_complete()` in `backend/main.py:99-137`) selects a
`UserProgress` where `user_id` and `step_id` match and inserts a new row when none is
found; `_record_attempt()` (`backend/main.py:10215-10227`), called from
`/api/exercises/validate` at `backend/main.py:1437-1487`, uses the same pattern.
3. Run the backend against PostgreSQL by setting `DATABASE_URL` to a Postgres URL (handled
in `backend/database.py:30-50`), and send two concurrent requests for the same `(user_id,
step_id)` pair (e.g., two overlapping `/api/progress/complete` calls or two
`/api/exercises/validate` submissions) so that both transactions execute their initial
SELECT before either INSERT commits.
4. Because there is no unique constraint on `(user_id, step_id)`, both transactions see
"no row", both execute `db.add(UserProgress(...))`, and both inserts succeed, leaving
duplicate `UserProgress` rows for that `(user_id, step_id)`; downstream consumers like
`get_progress()` (`backend/main.py:23-56`, which maps `step_id` to a single row) and
`_record_attempt()` (which increments only one row's `attempts`) then observe
non-deterministic state and inconsistent counts depending on which duplicate row they
happen to read.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/database.py
**Line:** 230:233
**Comment:**
*Race Condition: The `(user_id, step_id)` index is non-unique, but write paths use read-then-insert upsert logic for progress/attempts. Under concurrent requests, duplicate rows for the same user-step can be inserted, which inflates attempts/completions and breaks migration/aggregation assumptions. Enforce uniqueness with a unique constraint/index on `(user_id, step_id)`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
1 similar comment
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XL
Sequence DiagramThis diagram shows how a learner's exercise completion now flows from the web UI through validation, attempt counting, score normalization, and durable progress writes to support certificates and creator analytics. sequenceDiagram
participant Learner
participant WebApp
participant Backend
participant Database
Learner->>WebApp: Complete exercise step
WebApp->>Backend: Validate solution and record attempt
Backend->>Database: Increment attempt counter for user and step
Backend-->>WebApp: Validation result
WebApp->>Backend: Save completion with score
Backend->>Backend: Normalize score and select user or anon identity
Backend->>Database: Upsert user progress and issue certificate if eligible
Backend-->>WebApp: Progress saved and certificate info
Generated by CodeAnt AI |
Comment on lines
+2068
to
2070
| score = _normalize_score(body.get("score")) | ||
| response_data = body.get("response_data") | ||
| # 2026-04-25 — attribute progress to the logged-in user when possible. |
There was a problem hiding this comment.
🔴 Architect Review — CRITICAL
Step scores are now normalized to 0–1 at write time, but certificate issuance and the frontend still treat score as a percentage, so a perfect run can produce certificates showing scores like 1% instead of 100%.
Suggestion: Keep the 0–1 storage normalization, but convert back to a 0–100 scale when computing and returning certificate scores, or update all certificate consumers (including the frontend) to consistently interpret scores on a 0–1 scale in a coordinated change.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** backend/main.py
**Line:** 2068:2070
**Comment:**
*CRITICAL: Step scores are now normalized to 0–1 at write time, but certificate issuance and the frontend still treat `score` as a percentage, so a perfect run can produce certificates showing scores like `1%` instead of `100%`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+236
to
+247
| anon_id = request.cookies.get(ANON_COOKIE_NAME) | ||
| if not anon_id: | ||
| anon_id = uuid.uuid4().hex | ||
| request.state._issue_anon_cookie = True | ||
| request.state.anon_id = anon_id | ||
|
|
||
| response = await call_next(request) | ||
| if getattr(request.state, "_issue_anon_cookie", False): | ||
| # Mint the cookie on the way out so it's bound to the response | ||
| # the browser is about to receive. | ||
| _set_anon_cookie(response, anon_id) | ||
| return response |
There was a problem hiding this comment.
Suggestion: The middleware always re-issues sll_anon after call_next when the incoming request had no anon cookie, so on first-time /register or /login requests it can overwrite the route's delete_cookie and keep an anon cookie that was supposed to be cleared. Reset the issue flag (or skip issuing on auth success responses) when auth handlers explicitly clear the anon cookie. [logic error]
Severity Level: Major ⚠️
- ⚠️ Anon cookie persists after registration despite explicit clear.
- ⚠️ Pre-signup migration semantics harder to reason about.Steps of Reproduction ✅
1. The FastAPI app wires the auth middleware in `backend/main.py:277-285` via
`app.middleware("http")(_auth.session_middleware)`, ensuring every request passes through
`session_middleware` in `backend/auth.py:188-247`.
2. Send a first-time browser request to `POST /api/auth/register` (implemented at
`backend/auth.py:355-386`) without any `sll_anon` cookie set; in `session_middleware` at
`backend/auth.py:234-240`, `anon_id = request.cookies.get(ANON_COOKIE_NAME)` returns
`None`, so a new `uuid.uuid4().hex` is generated and `request.state._issue_anon_cookie` is
set to `True`.
3. Inside `register()` at `backend/auth.py:369-378`, the handler uses `anon_id =
getattr(request.state, "anon_id", None)` and then calls `_migrate_anon_progress`, after
which it calls `_clear_anon_cookie(response)` at `backend/auth.py:384` to delete the
`sll_anon` cookie on the outgoing response.
4. After the route handler returns, control resumes in `session_middleware` at
`backend/auth.py:242-247`; since `request.state._issue_anon_cookie` is still `True`,
`_set_anon_cookie(response, anon_id)` is called, re-adding a `Set-Cookie:
sll_anon=<anon_id>` header and effectively undoing the handler's explicit `delete_cookie`,
leaving an anon cookie present when the code intended it to be cleared.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/auth.py
**Line:** 236:247
**Comment:**
*Logic Error: The middleware always re-issues `sll_anon` after `call_next` when the incoming request had no anon cookie, so on first-time `/register` or `/login` requests it can overwrite the route's `delete_cookie` and keep an anon cookie that was supposed to be cleared. Reset the issue flag (or skip issuing on auth success responses) when auth handlers explicitly clear the anon cookie.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+371
to
+384
| anon_id = getattr(request.state, "anon_id", None) | ||
| if anon_id: | ||
| try: | ||
| n = await _migrate_anon_progress(db, anon_id, user.id) | ||
| if n: | ||
| logger.info("migrated %d anon progress rows from %s to user %s", n, anon_id, user.id) | ||
| except Exception: | ||
| logger.exception("anon-progress migration failed at register; continuing") | ||
| # Fresh session auto-logged-in. | ||
| sess = AuthSession.new_for_user(user.id, ttl_hours=SESSION_TTL_DAYS * 24) | ||
| db.add(sess) | ||
| await db.flush() | ||
| _set_session_cookie(response, sess.id) | ||
| _clear_anon_cookie(response) # subsequent requests use the session cookie |
There was a problem hiding this comment.
Suggestion: Anonymous-progress migration failures are swallowed, but the anon cookie is still cleared immediately after, which can permanently orphan guest progress/cert rows because future requests no longer carry the original anon identifier. Only clear the anon cookie after a successful migration (or keep it when migration fails so retry is possible). [incomplete implementation]
Severity Level: Critical 🚨
- ❌ Failed anon migration permanently strands guest progress data.
- ⚠️ Creator metrics miss some guest-to-user migrations.Steps of Reproduction ✅
1. Anonymous learners accumulate progress under their anon identifier by calling
`/api/progress/complete` in `backend/main.py:2055-2133`, which chooses `user_id =
getattr(request.state, "anon_id", None) or "default"` at `backend/main.py:23-27` when no
authenticated user exists and writes rows into `user_progress.user_id` for that anon_id.
2. The same browser later calls `POST /api/auth/register` handled by `register()` in
`backend/auth.py:355-386`; the middleware has already attached `request.state.anon_id` in
`session_middleware` at `backend/auth.py:234-240`, so `anon_id = getattr(request.state,
"anon_id", None)` at `backend/auth.py:371` resolves to the correct guest ID that matches
existing `UserProgress` and `Certificate` rows.
3. During registration, `_migrate_anon_progress(db, anon_id, user.id)` is invoked inside a
`try` block at `backend/auth.py:373-377`; if this raises (e.g., transient DB error or
constraint failure in the `select(_UP)` / `db.flush()` logic at
`backend/auth.py:283-352`), the `except` block logs `logger.exception("anon-progress
migration failed at register; continuing")` and then execution continues without
re-raising.
4. Regardless of whether `_migrate_anon_progress` succeeded or failed, the code always
calls `_clear_anon_cookie(response)` at `backend/auth.py:384`, removing the `sll_anon`
cookie; if migration failed, the existing `user_progress` and `certificates` rows remain
keyed by the now-lost anon_id, and future authenticated requests (which no longer carry
that anon cookie) cannot trigger `_migrate_anon_progress` again, permanently orphaning the
guest's progress and certificates.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/auth.py
**Line:** 371:384
**Comment:**
*Incomplete Implementation: Anonymous-progress migration failures are swallowed, but the anon cookie is still cleared immediately after, which can permanently orphan guest progress/cert rows because future requests no longer carry the original anon identifier. Only clear the anon cookie after a successful migration (or keep it when migration fails so retry is possible).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+772
to
+777
| sub_q = ( | ||
| select(_UP.user_id) | ||
| .where(_UP.step_id.in_(sids), _UP.completed.is_(True)) | ||
| .group_by(_UP.user_id) | ||
| .having(_func.count(_UP.step_id) >= len(sids)) | ||
| ) |
There was a problem hiding this comment.
Suggestion: Module completion uses count(step_id) >= len(sids) instead of counting distinct step IDs, so duplicated progress rows can falsely classify a learner as module-complete without finishing all different steps. Use count(distinct step_id) == len(sids) (or enforce uniqueness before counting). [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Module completion counts inflated by duplicate progress rows.
- ⚠️ Creator dashboard misidentifies modules learners actually finish.Steps of Reproduction ✅
1. Module completion within `creator_course_aggregate_stats()` in
`backend/auth.py:692-853` is computed in the per-module loop at `backend/auth.py:765-781`;
for each module's step IDs `sids`, the code builds `sub_q` at `backend/auth.py:772-777`
that groups by `_UP.user_id` and applies `.having(_func.count(_UP.step_id) >= len(sids))`,
counting non-distinct `UserProgress` rows.
2. The `UserProgress` table in `backend/database.py:203-233` has no uniqueness constraint
on `(user_id, step_id)`—`__table_args__` defines only non-unique indices at
`backend/database.py:230-233`—so multiple rows can exist for the same user-step pair.
3. Both `/api/progress/complete` in `backend/main.py:2055-2133` and `_record_attempt` in
`backend/main.py:10210-10233` use a read-then-insert upsert pattern
(`select(UserProgress).where(UserProgress.user_id == user_id, UserProgress.step_id ==
step_id)` followed by `db.add(UserProgress(...))` if no row) without any database-level
uniqueness; two concurrent requests for the same `(user_id, step_id)` can each observe "no
row", then both insert, creating duplicate completed rows for a subset of the module's
steps.
4. In such a duplicated state, a learner who has completed only some distinct steps in a
module can still satisfy `count(_UP.step_id) >= len(sids)` because duplicate rows for the
completed steps inflate the row count; `creator_course_aggregate_stats()` then classifies
that learner as module-complete and increments `mod_completed` at
`backend/auth.py:778-779`, overstating module completion for the creator dashboard.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** backend/auth.py
**Line:** 772:777
**Comment:**
*Incorrect Condition Logic: Module completion uses `count(step_id) >= len(sids)` instead of counting distinct step IDs, so duplicated progress rows can falsely classify a learner as module-complete without finishing all different steps. Use `count(distinct step_id) == len(sids)` (or enforce uniqueness before counting).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
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.
User description
Summary
Audited learner-progress tracking across the three interfaces (Web / Terminal CLI / VS Code) and landed the six correctness/leverage fixes called out in the audit. Five were correctness debt; the sixth (creator dashboard) leans on #3 + #4 to be meaningful.
Changes
#1 — Web
/progress/completeis now awaited, with retry queuefrontend/index.html. Was fire-and-forget (.catch(() => {})silently swallowed everything). Nowawaits the POST insidemarkStepComplete, and on failure pushes the entry to aPROGRESS_PENDING_KEYlocalStorage queue._drainPendingProgressWritesruns frominit()to flush pending writes on next load.#2 — Web fetches
/progress/{course_id}on enter-coursefrontend/index.html. Was localStorage-only — sign in on a new device showed zero progress. New_mergeBackendProgressIntoLocal(courseId)runs fromenterCoursebefore render and writes backend completion state into the local cache so the UI reflects durable progress.#3 — Score normalization to 0-1 at write-time
backend/main.py. New_normalize_score(raw)helper. CLI/VSCode send int 0-100, Web sends raw 0-1,UserProgress.scoreended up on mixed scales. Heuristic: 0-1 unchanged, >1 and ≤100 divided by 100, clamped to [0, 1]. Wired intoPOST /api/progress/complete.#4 —
UserProgress.attemptsserver-side counterbackend/database.py+backend/main.py. New column added via_ensure_column.POST /api/exercises/validatenow calls_record_attempt(db, user_id, step_id)on every grader invocation. Without this, per-step pass-rate / dropout signals don't exist for the creator dashboard.#5 — Anonymous → registered progress migration
backend/auth.py. Newsll_anoncookie (1y httpOnly), issued bysession_middlewareon first request when no auth session exists. Stored onrequest.state.anon_idand used as the fallbackuser_idfor/progress/*and/exercises/validate(replaces the shared"default"bucket — guests previously shared progress with each other). New_migrate_anon_progressruns at/auth/registerAND/auth/login; transfersUserProgress+Certificaterows, merges on conflict (max score, sum attempts, OR completed), deletes anon rows after merge, clears the anon cookie.#6 — Creator aggregate dashboard
backend/auth.py+frontend/index.html. NewGET /api/auth/creator/courses/{id}/aggregate-stats(auth-scoped to course creator) returns:Frontend:
loadCreatorLearnersForCoursenow fetches/aggregate-statsin parallel with/enrolled-learners. NewrenderCreatorAggregateOverviewrenders metrics cards + activity bar + per-step heatmap (red <40 % / amber <70 % / green ≥70 % pass rate) above the existing learner table. Aggregate-stats failure is non-fatal — table still renders.Test plan
End-to-end in-process smoke against a fresh SQLite DB:
_record_attempt × 3→UserProgress.attempts == 3✓_normalize_score(85)→0.85;(0.95)→0.95;(150)→1.0;(None)→None✓_migrate_anon_progress: anon row transferred + merged onto user_id with attempts/score/completed preserved; anon row deleted ✓/api/auth/creator/courses/{id}/aggregate-statsroute registered cleanlyManual on web after merge:
UserProgress.attemptsincrements🤖 Generated with Claude Code
CodeAnt-AI Description
Keep learner progress durable and show creators real course engagement data
What Changed
Impact
✅ Fewer lost progress updates✅ Progress follows learners across devices✅ Guest work is preserved after sign-up✅ Clearer creator insight into weak steps and learner drop-off🔄 Retrigger CodeAnt AI Review
Details
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.