Skip to content

Fix/1259 pi websocket state#1264

Open
sethbern wants to merge 5 commits into
RunestoneInteractive:mainfrom
sethbern:fix/1259-pi-websocket-state
Open

Fix/1259 pi websocket state#1264
sethbern wants to merge 5 commits into
RunestoneInteractive:mainfrom
sethbern:fix/1259-pi-websocket-state

Conversation

@sethbern

Copy link
Copy Markdown
Contributor

Fixes #1259.

This stores the current PI phase in Redis on each phase change and exposes it via /peer/api/current_state. On websocket connect/reconnect the client fetches and re-applies the phase, so students who briefly disconnect see the correct UI. A _catchup flag prevents side effects (no double vote counts, no enableNext navigation).

This also works if a student reloads the page, though I think they should still be discouraged to do that.

I was not sure the best way to test since I cant organically recreate the issue but I tested locally by using ws.close() in the console broweser and a full page reload mid-session. If you have any suggestions on testing it I would be open to trying @bnmnetp.

@sethbern sethbern requested a review from bnmnetp as a code owner June 29, 2026 15:05
@bnmnetp

bnmnetp commented Jun 29, 2026

Copy link
Copy Markdown
Member

A couple of drastic things to try:

  1. turn your wifi off and back on again.
  2. docker compose restart book to restart the book server.

@sethbern

Copy link
Copy Markdown
Contributor Author

I tested docker compose restart book and the catch-up works correctly on reconnect. I could not test wifi off/on locally since the app runs on localhost, but the catch-up logic triggers on any ws.onopen event regardless of disconnect reason, so it should work there as well.

@bnmnetp

bnmnetp commented Jun 29, 2026

Copy link
Copy Markdown
Member

I haven't studied your code, but the question I had was how are you distinguishing between a catch up and a legit new session?

Also, do we need to be concerned about Redis hygiene and have a mechanism to clean up old outdated state?

@bnmnetp

bnmnetp commented Jun 29, 2026

Copy link
Copy Markdown
Member

PS - I should have said this right up front, but this is great to see! Thanks.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses missed Peer Instruction UI state after transient WebSocket disconnects by persisting the “current phase” in Redis and having clients fetch/re-apply that phase on (re)connect to catch up.

Changes:

  • Client: on WebSocket open, fetch /assignment/peer/api/current_state and re-dispatch the returned control message with a _catchup flag to avoid side effects.
  • Server: persist the last phase-change control message in Redis (current_phase) and expose it via GET /peer/api/current_state.
  • Guard side effects during catch-up (e.g., prevent double vote counting and block enableNext navigation).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
components/rsptx/templates/staticAssets/js/peer.js Adds reconnect catch-up logic and _catchup guards in control-message handlers.
bases/rsptx/assignment_server_api/routers/peer.py Stores last PI phase in Redis and introduces /peer/api/current_state endpoint.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread components/rsptx/templates/staticAssets/js/peer.js Outdated
Comment thread bases/rsptx/assignment_server_api/routers/peer.py Outdated

@bnmnetp bnmnetp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the assignment_id isn't important, but maybe when the instructor starts a new PI session it should initialize the state? This limits us to one PI session per course, which is already a hard limitation, so not a critique of this PR.

Comment thread bases/rsptx/assignment_server_api/routers/peer.py
@sethbern

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review! I addressed these in the most recent pushes:

  • The _catchup flag is set client-side only when replaying the stored phase, so live socket messages are unaffected. For a new session, get_peer_dashboard already resets current_phase to enableNext so stale state clears automatically.
  • Switched to per-student storage keyed by assignment_id which also fixes the Copilot catch on A/B mode, where text-chat and verbal students get different phases (enableChat vs enableFaceChat) so a single course-wide key was wrong.
  • Also fixed the Copilot finding on enableChat so partner rows are no longer cleared on catch-up.

@bnmnetp

bnmnetp commented Jun 29, 2026

Copy link
Copy Markdown
Member

I'll plan to get this merged tomorrow, that should allow for some more widespread testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Runestone Issue: Peer Instruction - student occasionally doesn't see partner picker or chat panel

3 participants