From cfbda260fa1e00459c551697f64e168895719d0b Mon Sep 17 00:00:00 2001 From: ameer Date: Sat, 2 May 2026 22:40:52 +0800 Subject: [PATCH] fix: soft-reset UX + stale-cookie handling + leaderboard 'is_you' by id Three coupled fixes from the first manual test pass: 1. Stale signed cookie no longer 500s. `rooms.me()` now raises KeyError when the participant row is gone (the previous code deref'd None and threw TypeError, caught by quiz.js's catch-all as 'link expired'). `/api/session/{sid}/me` translates KeyError into 401 + delete_cookie, so the client falls back to the join form cleanly. Returning a JSONResponse directly because `raise HTTPException` discards Response.delete_cookie mutations (FastAPI middleware composes a fresh response on exception). 2. Reset is now a soft restart from the student's perspective. Before closing each student WS in `RoomManager.reset`, the server now sends a `{"type": "session_reset"}` message. The student SPA tears down local state and re-runs boot(); /me returns 401 (now that the participant is gone) and the join form renders without the user having to manually reload. The WS close handler suppresses its "Disconnected" screen during a reset to avoid a flash. 3. "You" highlight on the student leaderboard is now matched by id, not by name. `RoomManager.leaderboard()` accepts an optional `you_student_id` and stamps `is_you: true` on the matching entry only. No other students' ids leak over the wire (we still don't include `student_id` in the public top5 payload). quiz.js's renderBoard prefers `r.is_you` when any row is marked, falling back to name match for backward compatibility. 41/41 tests pass. Two new tests cover (a) the 401 + cookie-clear path after reset and (b) `is_you` marking only the requesting student. --- app/room.py | 39 ++++++++++++++++++++++++++++++------ app/routes_student.py | 14 +++++++++++-- static/quiz.js | 39 +++++++++++++++++++++++++++++++----- tests/test_api_student.py | 42 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 121 insertions(+), 13 deletions(-) diff --git a/app/room.py b/app/room.py index 97704b8..e4bf249 100644 --- a/app/room.py +++ b/app/room.py @@ -130,7 +130,14 @@ class RoomManager: (sid,), ) await db.commit() + # Tell each student client the session was reset BEFORE closing the + # socket, so the JS can clear local state and re-bootstrap into the + # join form rather than showing a generic "disconnected" screen. for ws in list(self.student_clients.get(sid, {}).keys()): + try: + await ws.send_json({"type": "session_reset"}) + except Exception: + pass try: await ws.close(code=4002) except Exception: @@ -504,13 +511,14 @@ class RoomManager: async def question_closed_message(self, sid: str, question_idx: int, identity: dict[str, Any] | None = None) -> dict[str, Any]: pool = await self.get_pool_for_session(sid) question = get_question(pool, question_idx) + you_id = identity["student_id"] if identity else None msg = { "type": "question_closed", "question_idx": question_idx, "correct": question["correct"], "explanation": question.get("explanation", ""), "histogram": await self.histogram(sid, question_idx), - "top5": await self.leaderboard(sid, limit=5), + "top5": await self.leaderboard(sid, limit=5, you_student_id=you_id), } if identity: student = identity["student_id"] @@ -528,14 +536,16 @@ class RoomManager: return msg async def between_message(self, sid: str, next_idx: int, identity: dict[str, Any] | None = None) -> dict[str, Any]: - msg = {"type": "between_questions", "next_idx": next_idx, "top5": await self.leaderboard(sid, limit=5)} + you_id = identity["student_id"] if identity else None + msg = {"type": "between_questions", "next_idx": next_idx, "top5": await self.leaderboard(sid, limit=5, you_student_id=you_id)} if identity: msg["your_rank"] = await self.rank_for(sid, identity["student_id"]) msg["your_total"] = await self.total_for(sid, identity["student_id"]) return msg async def ended_message(self, sid: str, identity: dict[str, Any] | None = None) -> dict[str, Any]: - msg = {"type": "session_ended", "final_top5": await self.leaderboard(sid, limit=5)} + you_id = identity["student_id"] if identity else None + msg = {"type": "session_ended", "final_top5": await self.leaderboard(sid, limit=5, you_student_id=you_id)} if identity: student = identity["student_id"] msg.update(await self.student_summary(sid, student)) @@ -564,7 +574,17 @@ class RoomManager: result["pending"] = max(0, int(total_row["count"]) - submitted - result["missed"]) return result - async def leaderboard(self, sid: str, limit: int | None = None, include_ids: bool = False) -> list[dict[str, Any]]: + async def leaderboard( + self, + sid: str, + limit: int | None = None, + include_ids: bool = False, + you_student_id: str | None = None, + ) -> list[dict[str, Any]]: + """Top scores. If `you_student_id` is given and that student appears + in the returned slice, that one entry is marked with `is_you: True` + so the client can highlight by id without exposing other students' + ids over the wire.""" query_limit = "" if limit is None else f"LIMIT {int(limit)}" async with connect(self.settings.db_path) as db: cursor = await db.execute( @@ -585,6 +605,8 @@ class RoomManager: item = {"rank": rank, "name": row["name"], "score": int(row["score"])} if include_ids: item["student_id"] = row["student_id"] + if you_student_id is not None and row["student_id"] == you_student_id: + item["is_you"] = True board.append(item) return board @@ -656,6 +678,11 @@ class RoomManager: async with connect(self.settings.db_path) as db: part_cursor = await db.execute("SELECT * FROM participants WHERE sid = ? AND student_id = ?", (sid, student_id)) participant = await part_cursor.fetchone() + if participant is None: + # Participant row is gone (typically because the instructor + # ran a reset). Caller is expected to translate this into a + # 401 + cookie-clear so the client lands on the join form. + raise KeyError(f"No participant {student_id!r} in session {sid!r}") sub_cursor = await db.execute( "SELECT question_idx, answer, elapsed_ms, score, status FROM submissions WHERE sid = ? AND student_id = ? ORDER BY question_idx", (sid, student_id), @@ -677,7 +704,7 @@ class RoomManager: "response_time_avg_ms": None, "response_time_distribution": {}, "average_score": 0, - "top5": await self.leaderboard(sid, limit=5), + "top5": await self.leaderboard(sid, limit=5, you_student_id=student_id), "your_rank": None, } async with connect(self.settings.db_path) as db: @@ -705,7 +732,7 @@ class RoomManager: "response_time_avg_ms": round(sum(times) / len(times)) if times else None, "response_time_distribution": distribution, "average_score": round(sum(scores) / len(scores), 2) if scores else 0, - "top5": await self.leaderboard(sid, limit=5), + "top5": await self.leaderboard(sid, limit=5, you_student_id=student_id), } if student_id: payload["your_rank"] = await self.rank_for(sid, student_id) diff --git a/app/routes_student.py b/app/routes_student.py index 9a4ba5d..12d81c6 100644 --- a/app/routes_student.py +++ b/app/routes_student.py @@ -6,7 +6,7 @@ from pathlib import Path from uuid import uuid4 from fastapi import APIRouter, HTTPException, Request, Response, WebSocket -from fastapi.responses import FileResponse, HTMLResponse, RedirectResponse +from fastapi.responses import FileResponse, HTMLResponse, JSONResponse, RedirectResponse from app import auth from app.config import Settings @@ -70,7 +70,17 @@ def router(settings: Settings, rooms: RoomManager) -> APIRouter: identity = auth.get_student_identity(settings, request, sid) if not identity: raise HTTPException(status_code=401, detail="Student cookie required") - return await rooms.me(sid, identity["student_id"]) + try: + return await rooms.me(sid, identity["student_id"]) + except KeyError: + # Cookie's student_id is no longer in the DB (e.g. session reset + # or DB rebuilt while the cookie persisted). Send 401 with the + # cookie cleared so the client renders the join form. We build + # the JSONResponse directly because raising HTTPException would + # bypass the cookie mutation. + resp = JSONResponse({"detail": "Re-join required"}, status_code=401) + resp.delete_cookie(auth.STUDENT_COOKIE, path="/") + return resp @api.get("/api/session/{sid}/stats") async def stats(sid: str, request: Request, question_idx: int | None = None): diff --git a/static/quiz.js b/static/quiz.js index db563fc..8796180 100644 --- a/static/quiz.js +++ b/static/quiz.js @@ -130,6 +130,10 @@ function connect() { try { handleMessage(JSON.parse(event.data)); } catch (e) { console.warn("bad ws msg", e); } }); ws.addEventListener("close", () => { + // session_reset already drove a re-boot; suppress the generic + // "disconnected" screen so it doesn't briefly flash on top of the + // "Re-joining…" interstitial. + if (store.resetting) return; stopCountdown(); setView(`
@@ -149,10 +153,32 @@ function handleMessage(message) { case "question_closed": return renderReveal(message); case "between_questions": return renderBetween(message); case "session_ended": return renderFinished(message); + case "session_reset": return handleSessionReset(); case "error": return renderError(message); } } +function handleSessionReset() { + // Instructor cleared everyone out. Tear local state down and re-boot; + // /api/session//me will return 401 (with cookie cleared by the + // server) and we'll land cleanly on the join form. + store.resetting = true; + stopCountdown(); + store.me = null; + store.currentQuestion = null; + store.submitted = null; + store.pickedAnswer = null; + if (store.ws) { try { store.ws.close(); } catch {} store.ws = null; } + setView(` +
+

Session reset

+

Your instructor reset the session. Re-joining…

+ +
+ `); + setTimeout(() => { store.resetting = false; boot(); }, 600); +} + function renderState(message) { store.currentQuestion = null; store.submitted = null; @@ -314,15 +340,18 @@ function renderFinished(message) { function renderBoard(rows = []) { if (!rows || !rows.length) return `

No scores yet.

`; - // The server's student-facing top5 doesn't include student_id, so match - // on display name. Same-name collisions are rare in a single classroom - // session; if it ever happens both rows highlight, which still reads - // as "yours might be one of these" to the student. + // The server marks the requesting student's row with `is_you: true` so + // we can highlight by id without other students' ids ever crossing the + // wire. Falls back to name match only if the server didn't mark anything + // (older payloads pre-migration). + const anyMarked = rows.some((r) => r.is_you); const myName = store.me?.name; return `
    ${rows.map((r) => { - const isYou = myName && r.name && r.name === myName; + const isYou = anyMarked + ? !!r.is_you + : (myName && r.name && r.name === myName); return `
  1. ${r.rank} diff --git a/tests/test_api_student.py b/tests/test_api_student.py index f983118..88c4698 100644 --- a/tests/test_api_student.py +++ b/tests/test_api_student.py @@ -27,6 +27,48 @@ def test_root_without_sid_redirects_to_canonical(client, sid): assert response.headers["location"] == f"/?sid={sid}" +def test_me_returns_401_and_clears_cookie_when_participant_is_gone(client, sid): + """A stale signed cookie (e.g. after admin reset wiped participants) must + return 401 with the cookie cleared, not 500. The client uses 401 to fall + back to the join form.""" + join_student(client, sid, "s1", "Student One") + assert client.get(f"/api/session/{sid}/me").status_code == 200 + + # Simulate the post-reset state: cookie still valid by signature, + # but the participant row is gone. + rooms = client.app.state.rooms + client.portal.call(rooms.reset, sid) + + response = client.get(f"/api/session/{sid}/me") + assert response.status_code == 401 + # Server should send a Set-Cookie that clears the qz_student cookie. + assert any( + h.lower() == "set-cookie" and "qz_student" in v and ('Max-Age=0' in v or 'expires=' in v.lower()) + for h, v in response.headers.items() + ), response.headers + + +def test_leaderboard_marks_requesting_student_with_is_you(client, sid): + """The student-facing top5 should mark only the requesting student's row + with `is_you: true`, never include other students' ids.""" + rooms = client.app.state.rooms + join_student(client, sid, "s1", "Alice") + join_student(client, sid, "s2", "Bob") + client.portal.call(rooms.open_question, sid, 0, 5) + client.portal.call(rooms.submit_answer, sid, "s1", 0, "B") + client.portal.call(rooms.submit_answer, sid, "s2", 0, "B") + client.portal.call(rooms.close_question, sid) + + # Stats endpoint reflects the requesting student's identity from cookie. + stats = client.get(f"/api/session/{sid}/stats?question_idx=0").json() + you_rows = [r for r in stats["top5"] if r.get("is_you")] + other_rows = [r for r in stats["top5"] if not r.get("is_you")] + assert len(you_rows) == 1 + assert you_rows[0]["name"] in {"Alice", "Bob"} + # Other students' ids are not exposed. + assert all("student_id" not in r for r in other_rows) + + def test_invalid_session_and_missing_cookie_paths(client): response = client.get("/?sid=BAD") assert response.status_code == 404