ChatDO Implementation #71
Open
Ananya44444 wants to merge 4 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 14 minutes and 23 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR moves classroom chat to a dedicated ChangesDedicated chat WebSocket channel
Sequence Diagram(s)sequenceDiagram
participant Client as classroom_poc.html
participant Worker as src/worker.py
participant ChatDO
participant DB as chat_message table
Client->>Worker: open /ws/chat/:room_id
Worker->>ChatDO: forward WebSocket upgrade
ChatDO->>DB: load recent rows
ChatDO-->>Client: chat_history
Client->>ChatDO: chat_message
ChatDO->>DB: persist row
ChatDO-->>Client: broadcast chat_message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/classroom_poc.html`:
- Around line 691-695: chatSend currently swallows messages when chatWs is
reconnecting or closed, so the submit flow thinks the send succeeded. Update
chatSend to return a boolean indicating whether chatWs.send(JSON.stringify(obj))
actually ran, and adjust the submit handler that calls it to keep the input
populated and show a user-facing failure message when it returns false. Use the
existing chatSend function and the chat submit handler in classroom_poc.html to
locate the fix, including the duplicate call site noted in the review.
- Around line 641-645: The WebSocket URL construction in classroom_poc.html is
exposing the bearer token via the query string, which should be removed. Update
the chat connection flow around the URL assembly for the WebSocket endpoint so
it no longer appends token/edu_token to params; instead use a short-lived
server-issued chat ticket or a server-validated subprotocol/cookie-based auth
path before creating the socket. Keep the user_id, display_name, and
classroom_id parameters only if needed for non-sensitive routing, and adjust the
websocket open logic accordingly.
- Around line 644-645: The chat websocket URL is sending a separate
client-controlled classroom_id alongside the /ws/chat/:id route, which can
diverge from the actual room being routed. Update the chat connection flow so
ChatDO derives the classroom identifier from the route/path value only and no
longer reads or persists history from a query string classroom_id. Remove the
extra params.set('classroom_id', roomId) usage in the public/classroom_poc.html
connection setup and ensure the server-side handling around ChatDO uses the
routed id as the single source of truth.
- Around line 654-668: The WebSocket chat update handlers in the chat message
flow append content dynamically, but the chat log is not exposed as an
announceable ARIA region. Update the chat log markup used by
appendChatMessage/chatMessages in classroom_poc.html to use an appropriate live
region (such as role="log" with live/polite behavior and relevant
atomic/relevant settings) so screen readers announce new chat_history and
chat_message entries as they arrive.
In `@src/chat_do.py`:
- Around line 180-185: The room identifier in ChatDO is being taken from a
client-controlled query string, which allows mismatched room identity between
the routed Durable Object and the persisted chat history. Update ChatDO to
derive classroom_id from the routed path/DO identity used by the
/ws/chat/<room_id> entrypoint instead of parse_qs(request.url), and propagate
that trusted value through the startup/load/save paths in the affected ChatDO
methods so all reads and writes stay bound to the same room.
- Around line 294-303: The chat flow in send/publish logic broadcasts with
_broadcast before _persist_message completes, so messages can appear live even
when encrypted durable storage fails. Update the caller around _broadcast and
_persist_message so persistence is attempted and verified first; only call
_broadcast after successful encrypted DB write, and make _persist_message
surface missing encryption config or DB/encryption failures instead of
swallowing them. Apply the same ordering/failure handling in the other affected
message path that uses the same pattern.
- Around line 151-173: The _load_history method in ChatDO currently loads the
oldest rows and marks history as loaded before the database and decrypt work
succeeds. Update the query so it fetches the latest _MAX_BUFFER messages in
reverse order and then restores chronological order for self.messages, and move
the _history_loaded assignment to after all rows are successfully fetched and
decrypted. Keep the fix localized to _load_history and preserve the existing
retry behavior by leaving the flag false when an exception occurs.
- Around line 71-87: Malformed v1 ciphertext is escaping the existing error
handling in decrypt_aes, causing _load_history() to fail the entire room load.
Move the base64 decoding and IV/ct extraction for the AES-GCM path inside the
try block in decrypt_aes so any bad row is caught there and returns "[decryption
error]" instead of propagating; keep the current fallback behavior for non-v1
ciphertext and preserve the _load_history() flow unchanged.
In `@src/worker.py`:
- Around line 733-734: Drop the redundant chat_message classroom-only index from
the schema setup in the worker’s database initialization where the index list is
defined, and keep the composite idx_chat_created index as the single supporting
index for classroom_id lookups. Update the migration/creation logic around the
chat_message index definitions so only the composite index remains, preserving
the existing access pattern while avoiding unnecessary write overhead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea842fb1-e9e7-415d-88e6-dd764c80359a
⛔ Files ignored due to path filters (1)
migrations/0007_add_chat_message.sqlis excluded by!**/migrations/**
📒 Files selected for processing (4)
public/classroom_poc.htmlsrc/chat_do.pysrc/worker.pywrangler.toml
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/chat_do.py (1)
353-355: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winLog missing chat encryption configuration.
When
CHAT_ENCRYPTION_KEY/ENCRYPTION_KEYis absent, every send fails, but the server records no actionable diagnostic. Emit a clear non-secret log message or fail the WebSocket upgrade earlier.🩺 Proposed diagnostic
enc_key = getattr(self.env, "CHAT_ENCRYPTION_KEY", None) or getattr(self.env, "ENCRYPTION_KEY", "") if not enc_key: + print("[ChatDO._persist_message] CHAT_ENCRYPTION_KEY is not configured") return FalseAs per path instructions, Python code should include proper error handling and logging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/chat_do.py` around lines 353 - 355, The chat encryption lookup in the validation path is silently returning False when both CHAT_ENCRYPTION_KEY and ENCRYPTION_KEY are missing, so add an actionable non-secret log message or surface the failure during the WebSocket upgrade. Update the logic around the encryption check in the method that reads self.env (and any caller handling this return) to record a clear diagnostic before returning, or raise an early upgrade error, so operators can tell why sends are failing without exposing the key.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/classroom_poc.html`:
- Around line 642-645: The chat WebSocket URL construction is relying on
client-controlled query parameters for identity, which allows spoofing of chat
participants. Update the connection flow in the chat URL builder and the related
ChatDO handshake so authentication comes from a server-validated session,
ticket, cookie, or subprotocol instead of user_id/display_name query params, and
derive those fields server-side before accepting any chat frames.
In `@src/chat_do.py`:
- Around line 197-209: The chat room entry logic in the websocket handler
currently trusts client-supplied user_id/display_name when no token is present,
which allows unauthorized access and impersonation. Update the path in the
websocket request handling around verify_token, classroom_id extraction, and the
_load_history call so that a valid JWT is required, malformed /ws/chat/:id paths
are rejected, and the authenticated user is checked for access to classroom_id
before any history is loaded or messages are accepted. Also ensure the classroom
client sends the auth token instead of relying on query-only identity fields.
In `@src/worker.py`:
- Around line 53-60: The ChatDO import fallback in src/worker.py is too broad
and the DurableObject fallback class has a typo that will raise a NameError.
Update the _ChatDO import chain to catch only the missing-module case for
chat_do and src.chat_do so real import-time errors still surface, and replace
the invalid placeholder body in the fallback DurableObject class so it is a
valid no-op definition.
---
Duplicate comments:
In `@src/chat_do.py`:
- Around line 353-355: The chat encryption lookup in the validation path is
silently returning False when both CHAT_ENCRYPTION_KEY and ENCRYPTION_KEY are
missing, so add an actionable non-secret log message or surface the failure
during the WebSocket upgrade. Update the logic around the encryption check in
the method that reads self.env (and any caller handling this return) to record a
clear diagnostic before returning, or raise an early upgrade error, so operators
can tell why sends are failing without exposing the key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d678753d-f444-4484-979a-28b63d9a3ae9
📒 Files selected for processing (3)
public/classroom_poc.htmlsrc/chat_do.pysrc/worker.py
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.
Overview
This PR extracts chat functionality from
ClassroomDOinto a dedicatedChatDO, aligning with the Virtual Classroom specification. It introduces persistent, encrypted chat with proper WebSocket routing and frontend separation.Key Changes
1. ChatDO (new Durable Object)
Handles all chat-related functionality:
/ws/chat/:id)ctx.getWebSockets()Persists messages to D1 with AES-256-GCM encryption
Replays chat history on client reconnect
2. D1 Schema & Migrations
chat_messagetable (0007_add_chat_message.sql)_DDLfor fresh environment initialization3. Frontend Integration
chatWsconnectionconnectChatWS()chatSend()4. Routing Updates
Added
/ws/chat/:idroute in workerUpdated
run_worker_first:to ensure WebSocket routes reach the Worker
5. ClassroomDO Cleanup
chat_messagecase deprecated (no-op)Result
Chat is now:
Migration Notes
ensure:
This PR moves chat functionality into a dedicated
ChatDODurable Object (separate fromClassroomDO) to align with the Virtual Classroom specification and isolate chat signaling from classroom traffic.Key changes:
/ws/chat/:id, updates worker routing (run_worker_firstto include/ws/*), and forwards WebSocket requests to theCHAT_DObinding."chat_message"handling fromClassroomDO(now treated as a no-op there) so chat messages flow exclusively through the chat route/DO.chat_messagetable (0007_add_chat_message.sql) and updates the worker DDL/migrations for fresh environments.CHAT_ENCRYPTION_KEYis configured, loads recent history on attach, and restores/binds active WebSocket sessions usingctx.getWebSockets()for hibernation resilience.Frontend impact:
connectChatWS()and sends messages viachatSend()instead of using the classroom socket.chat_history,chat_message) received from the chat socket; accessibility markup for the message log is improved.Migration/ops note:
CHAT_ENCRYPTION_KEYmust be set (e.g.,wrangler secret put CHAT_ENCRYPTION_KEY).