feat: Add waiting room DO and courses flows across learner and host journeys - #49
Open
Lakshya-2440 wants to merge 4 commits into
Open
feat: Add waiting room DO and courses flows across learner and host journeys#49Lakshya-2440 wants to merge 4 commits into
Lakshya-2440 wants to merge 4 commits into
Conversation
WalkthroughThis pull request adds waiting-room listing and detail workflows, course creation and browsing pages, an activity detail view, shared browser authentication utilities, a Durable Object backend, and authenticated deletion APIs for activities and sessions. ChangesWaiting room workflows
Course management views
Shared authentication
Activity deletion controls
Estimated code review effort: 4 (Complex) | ~55 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 20
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@public/course.html`:
- Line 119: The esc function only escapes &, <, > which is unsafe for values
interpolated into quoted HTML attributes (see its use in building the delete
button onclick with deleteSession('\'+ esc(s.id) +\'')). Update esc to also
escape both single-quote (') and double-quote (") characters (e.g., to ' and
") so attribute contexts are safe, and align its behavior with the existing
escapeHtml used in courses.js / waitingroom.js so a future shared escape helper
can be introduced; change the esc implementation and verify uses like
deleteSession( ... esc(s.id) ...) and any other attribute or JS-embedded
interpolations still work with the hardened escaping.
- Around line 144-168: deleteCourse and deleteSession are async functions
invoked from inline onclick handlers, so thrown errors become unhandled promise
rejections and give no user feedback; wrap the fetch/response logic in each
function in a try/catch (same pattern used in joinActivity) and on error surface
the message to the user (e.g., alert(err.message || 'Failed to delete
course/session')) instead of throwing, while preserving the existing redirects
and successful-path behavior; update both deleteCourse() and
deleteSession(sessionId) accordingly.
In `@public/courses.html`:
- Around line 50-53: The search input inside the .nav-search div lacks an
accessible label; update the <input> with a programmatic label by either
wrapping it in a <label> element or adding an appropriate aria-label (e.g.,
aria-label="Search courses" or aria-label describing purpose) so screen readers
can identify it — target the input element within the .nav-search container to
apply this change.
In `@public/courses.js`:
- Around line 1-22: The three functions readAuthValue, clearAuth, and
authHeaders are duplicated across courses.js, waitingroom.js,
waitingroom-detail.js and inlined in course.html; extract them into a single new
public/auth.js that defines these functions once and exposes them globally
(e.g., attach to window or export for module use), include public/auth.js before
the other page scripts, then remove the duplicate implementations from
courses.js, waitingroom.js, waitingroom-detail.js and the inline script in
course.html so all callers use the shared readAuthValue, clearAuth, and
authHeaders implementations.
- Around line 119-127: The code dereferences DOM nodes grid
(document.getElementById('courses')) and empty
(document.getElementById('empty')) without checking they exist, causing a
TypeError if either is missing; update the init logic around fetchCourses/const
grid/const empty to guard against null: verify empty and grid are non-null
before accessing style or innerHTML (e.g., return or skip the DOM updates when
one is missing), and ensure the behavior of the early-return branch (when
courses.length === 0) handles the missing elements consistently so no property
is accessed on a null reference.
- Around line 81-97: In renderSessions, time and sub are being collapsed into
one expression so sub (location/description or locked message) is never shown
when time exists; update renderSessions so you render time and sub as separate
lines: compute title, time and sub as you already do, then in the template
output a session-title, a session-time line that uses escapeHtml(time) only when
time is truthy, and a separate session-sub line that uses escapeHtml(sub) (or
the locked message) so sub is shown even when time exists; ensure you use
escapeHtml for both time and sub and preserve the locked behavior.
In `@public/teach-course.html`:
- Around line 137-145: Replace the non-interactive <span class="linkish"
aria-disabled="true"> used for "+ Add Another Session Time" with a real
interactive element: change it to a <button type="button"> (or <button
type="button" disabled> if not yet implemented) so it is focusable,
keyboard-operable, and announced to assistive tech; ensure any existing click
handler in teach-course.js is bound to this button (refer to the ".linkish"
selector/handler and update it to target the new button). Also change the inputs
with id="subject" and id="tags" from type="text" to type="hidden" (or remove
display:none CSS in favor of type="hidden") so they are semantically correct and
avoid HTMLHint label warnings.
In `@public/teach-course.js`:
- Line 299: The call to fulfillRoom is using the DOM element textContent
('nav-uname') as the actor; instead use the authoritative profile data (e.g.
profile.username or the hoisted currentUser variable) instead of reading the
navbar. Update the await fulfillRoom(...) invocation to pass profile.username
(or currentUser if you move it out of the try-block started at line ~221) as the
actor argument while keeping waitingRoomId, course.id, and course.title || title
unchanged.
- Around line 136-192: The toolbar handler uses errEl
(document.getElementById('err-msg')) without validating it, so calls to
errEl.textContent can throw; update wireDescriptionToolbar to guard errEl the
same as toolbar and textarea (e.g., if (!toolbar || !textarea || !errEl) return)
or otherwise provide a safe fallback for errEl before adding the click listener
so that both uses of errEl.textContent (the initial clear in the click handler
and the 'help' branch) are safe; modify the function wiring
(wireDescriptionToolbar) to reference the validated errEl when invoking
replaceSelection/lineifySelection actions.
- Around line 271-307: The submit handler can create duplicate courses on retry
because createCourse is called unconditionally; update the handler (the function
that calls createCourse, createSession and fulfillRoom) to stash the created
course id and a session flag in local variables (e.g., courseId, sessionCreated)
and skip calling createCourse/createSession when those values exist (use
courseId to avoid re-creating the course and sessionCreated to avoid re-creating
the session), persist courseId to sessionStorage/localStorage so a page reload
still prevents duplicate creation, and on failure attempt a best-effort cleanup
by calling DELETE /api/activities/:id using the stored courseId before
re-enabling the submit button; reference createCourse, createSession,
fulfillRoom, waitingRoomId, btn and the submit handler in your changes.
In `@public/waitingroom-detail.js`:
- Around line 1-27: The auth helper functions (readAuthValue, writeAuthValue,
clearAuth, authHeaders) and related fetchRoom logic are duplicated across
multiple files; extract them into a single shared module (e.g.,
public/js/auth.js) that exports these functions and import/include that module
from public/waitingroom-detail.js (and the other files) via a <script
type="module"> or shared include; update waitingroom-detail.js to remove the
local definitions and instead call the imported
readAuthValue/writeAuthValue/clearAuth/authHeaders (and fetchRoom if
applicable), keeping exported function names unchanged so callers don’t need to
change.
In `@public/waitingroom.html`:
- Around line 27-34: The search input element (placeholder "What do you want to
learn?") is missing an accessible label and all icon buttons (elements with
class "icon-btn") lack explicit button types; add aria-label="Search" to that
input and add type="button" to each <button class="icon-btn"> (including the
cart button with the .cart-badge span) so intent is explicit and screen readers
can access the input.
- Around line 60-62: The initial HTML seeds for the room counter and username
are hard-coded causing a flash; change the default content of the elements with
id "room-count" and "nav-uname" in waitingroom.html from "8" and "lakshya" to
sensible placeholders (e.g., "0" for `#room-count` and "..." for `#nav-uname`) so
the first paint matches eventual JS-updated values (consistent with courses.html
/ teach-course.html); update the text nodes in the elements with id="room-count"
and id="nav-uname" accordingly.
In `@public/waitingroom.js`:
- Around line 330-346: The polling code uses setInterval(refreshRooms, 3000) and
only clears it on beforeunload and only guards re-entrancy with isRefreshing;
update this by (1) also listening for pagehide to clear the refreshInterval (add
a handler alongside window.addEventListener('beforeunload', ...)), and (2) make
the visibilitychange handler debounce against an in-flight or very-recent
refresh by checking isRefreshing before calling refreshRooms and/or scheduling a
short setTimeout (e.g., next tick/100ms) if isRefreshing is true so rapid
visibility flicker doesn't spur multiple calls; reference the functions/vars
refreshRooms, isRefreshing, refreshInterval and the events
visibilitychange/pagehide when locating the changes.
- Around line 143-198: The isCreator check uses username equality only; change
it to prefer comparing room.creatorId to currentParticipantId
(String(room.creatorId) === String(currentParticipantId)) and only fall back to
the existing username comparison (String(room.creator || '').toLowerCase() ===
String(currentUser).toLowerCase()) so the Delete button rendering (uses
isCreator) matches server-side creatorId logic and still works for
legacy/name-only rooms.
- Around line 143-155: The renderRooms function currently assumes DOM nodes
exist and will throw if document.getElementById returns null; update renderRooms
to guard accesses to grid, roomCount, youLabel and tabCount (e.g., if (!grid)
return; or only set textContent/innerHTML when the element is non-null) before
dereferencing .textContent or .innerHTML; likewise, add null checks before
setting nav-uname and nav-avatar in the code that updates the navigation (check
document.getElementById('nav-uname') and document.getElementById('nav-avatar')
exist before assigning .textContent or .src) so missing elements no longer crash
the page.
In `@src/worker.py`:
- Around line 82-99: The path-matching is too loose: the regex in m_room (used
when method == "GET") can accidentally match reserved sub-paths like
create/join/leave/delete/fulfill and the code uses
path.endswith('/api/waitingrooms') which can match unintended URLs; update the
m_room regex in the GET branch (the re.fullmatch call that assigns m_room) to
exclude those reserved words (e.g., use a negative lookahead for
create|join|leave|delete|fulfill) and replace any uses of
path.endswith('/api/waitingrooms') (and other similar endswith checks) with
exact equality checks (path == '/api/waitingrooms') so matching is unambiguous
and safe.
- Around line 186-198: The delete and fulfill handlers allow anonymous
deletes/fulfillments when a room lacks creator_id because the current checks
skip both branches if actor or creator are empty; fix by explicitly requiring an
authenticated actor and refusing when identity values are empty: in the delete
handler (variables: creator_id, actor, creator, normalized_actor,
normalized_creator, id_matches, name_matches) add an early check that auth_user
(or actor) must be present and non-empty and return 403 if missing, and treat
empty creator (legacy room with missing creator name) as not-authorized rather
than permitting deletion; apply the identical explicit
authentication-and-non-empty-identity check to the fulfill handler (the same
variable names and matching logic around name_matches) so anonymous callers
cannot operate on rooms without creatorId.
- Around line 43-234: The WaitingRoomDO handlers never run because there's no
durable object binding or dispatcher route: add a DO binding (e.g., WAITING_ROOM
= { class_name = "WaitingRoomDO" }) in wrangler.toml and update the _dispatch
function to detect /api/waitingroom* paths and forward them to the WaitingRoomDO
stub (create/get the stub and call fetch/handle as other DO routes do); also
move imports for urlparse and js to the top of the file per PEP8 (no late
imports) and add type annotations: change WaitingRoomDO.__init__(self, state,
env) to return -> None and annotate _normalize_participants(raw) with
appropriate parameter and return types (e.g., raw: Any -> List[Dict[str,str]])
to satisfy ANN204/ANN202.
- Around line 1409-1427: The deletion sequence uses multiple
env.DB.prepare(...).run() calls which are separate D1 transactions and can leave
partial state; replace with a single atomic batch by building the statements
(using env.DB.prepare(...) bound with act_id and user["id"]) and executing them
via env.DB.batch(to_js(stmts)) to ensure atomic cascade deletion; additionally,
after successful deletion invoke your waiting-room invalidation (either call the
Durable Object endpoint to clear courseId/courseTitle or emit an event so the
waiting room DO removes stale linked-course fields) so the waiting room does not
show links to deleted activities; keep the existing capture_exception(...) error
handling around the batch call to log failures.
🪄 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: cbaa49bf-36d5-4e73-8bd5-7b698f111dae
📒 Files selected for processing (10)
public/course.htmlpublic/courses.htmlpublic/courses.jspublic/teach-course.htmlpublic/teach-course.jspublic/waitingroom-detail.htmlpublic/waitingroom-detail.jspublic/waitingroom.htmlpublic/waitingroom.jssrc/worker.py
Introduces courses and waiting-room detail pages, teach-course flow wiring, and worker API support for room fulfillment plus host-managed course/session deletion.
Lakshya-2440
force-pushed
the
feat/waitingroom-courses-clean
branch
from
April 20, 2026 23:04
c994f61 to
9b05d86
Compare
There was a problem hiding this comment.
Pull request overview
This PR connects the “Waiting Room” demand signal to course creation and adds host management delete controls, enabling an end-to-end learner discovery → host fulfillment → course/session management flow.
Changes:
- Add a Waiting Room Durable Object with endpoints to create/list/get/join/leave/delete/fulfill rooms.
- Introduce new frontend pages/scripts for waiting room list/detail, course listing, and “teach course from waiting room”.
- Add backend delete endpoints for activities and sessions and wire corresponding UI controls.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
src/worker.py |
Adds WaitingRoomDO plus new /api/activities/delete and /api/sessions/delete endpoints. |
public/waitingroom.js |
Implements waiting room listing UI, join/leave/delete actions, and polling refresh. |
public/waitingroom.html |
Adds Waiting Rooms landing page markup and modal for creating requests. |
public/waitingroom-detail.js |
Implements waiting room detail rendering and actions (join/leave/teach/delete). |
public/waitingroom-detail.html |
Adds Waiting Room detail page markup/styles. |
public/teach-course.js |
Implements course creation from a waiting room + fulfillment call. |
public/teach-course.html |
Adds “teach course” form UI for fulfilling a waiting room. |
public/courses.js |
Implements authenticated course listing and session expansion. |
public/courses.html |
Adds Courses listing page markup and styling. |
public/course.html |
Adds course detail page UI with host-side delete controls for course/sessions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
@Lakshya-2440 please take a look |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/course.html`:
- Around line 148-150: Update the delete confirmation in deleteCourse and the
associated delete button label to use “activity” instead of “course,” preserving
the existing warning that all sessions are deleted and the action cannot be
undone.
- Line 118: Update the user initialization around
JSON.parse(readAuthValue('edu_user')) to catch malformed stored JSON instead of
allowing page initialization to throw. When parsing fails, clear both the
invalid edu_user value and its associated auth entry, then continue with a null
user so the activity page can load.
In `@public/courses.html`:
- Around line 54-60: Add descriptive aria-label attributes to each icon-only
button in the nav-icons container, naming the actions represented by 💬, 🛒, 🔔,
🌐, and 🌙. Keep the existing button types, icons, and cart-badge markup
unchanged.
In `@public/courses.js`:
- Around line 92-95: Update the session-loading handler in public/courses.js to
listen for the details toggle event rather than only
button[data-action="toggle"] clicks. In the toggle handler, detect when the
details element opens and invoke fetchCourseWithSessions using its
data-course-id, preserving the existing loading and already-loaded behavior for
both native summary clicks and programmatic details.open changes.
In `@public/waitingroom.css`:
- Line 10: Remove the quotes from the Inter font-family name in every
declaration in waitingroom.css, including all eight occurrences, while
preserving the existing fallback fonts and ordering.
In `@public/waitingroom.html`:
- Around line 108-111: Update the newsletter controls in the newsletter form:
add an aria-label to the email input and an aria-label plus type="button" to the
submit button. Keep the existing placeholder and button content unchanged.
In `@src/worker.py`:
- Around line 3274-3285: Update the delete flow in api_delete_session to execute
the session_attendance and sessions DELETE statements together through
env.DB.batch, following the existing batching pattern used by
api_delete_activity. Preserve the current parameter bindings, exception capture
via capture_exception, error response, and success response.
🪄 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: 9a5d3c16-9e9b-4994-a1ec-f7270d5caa7e
📒 Files selected for processing (13)
public/course.htmlpublic/courses.htmlpublic/courses.jspublic/js/auth.jspublic/teach-course.htmlpublic/teach-course.jspublic/waitingroom-detail.htmlpublic/waitingroom-detail.jspublic/waitingroom.csspublic/waitingroom.htmlpublic/waitingroom.jssrc/worker.pywrangler.toml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/worker.py (7)
7829-7837: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftNo change needed for the binding name.
Watch the global Durable Object throughput as concurrent viewers grow.
env.WAITING_ROOM_DOmatchesWAITING_ROOM_DOinwrangler.toml. The singleidFromName("global")instance also handles all read and write operations. If traffic reaches a Durable Object single-threaded bottleneck, split mutating operations such ascreate,join,leave,fulfill, anddeleteinto separate per-room Durable Objects and keep only the listing index in the global instance.🤖 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/worker.py` around lines 7829 - 7837, The current global WaitingRoomDO design serializes all room reads and writes through idFromName("global"), creating a potential throughput bottleneck. Monitor concurrency and, if saturation occurs, refactor the waiting-room Durable Object routing so create, join, leave, fulfill, and delete use separate per-room Durable Objects while the global instance retains only the listing index; keep the existing WAITING_ROOM_DO binding unchanged.
7519-7546: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftFilter and bound the message query instead of decrypting every record.
The query loads all
legacy_recordsrows for both participants, with noLIMITand no thread filter. The code then decrypts each row and discards the rows that belong to other conversations. The work grows with the total number of messages the two users have ever exchanged with anyone, not with the size of this thread. Each discarded row still costs one AES decryption.Two improvements:
- Add a
LIMITand pagination to the query, ordered bycreated_at. A thread view rarely needs the full history at once.- Store
thread_idas an indexed column on the message row at write time inapi_send_thread_message(lines 7415-7418), then filter on it in SQL. The value is already known when the record is inserted.Also, the
except Exception: passat lines 7545-7546 drops a message silently when decryption fails. Ruff reports this as S110. Log the failure withcapture_exceptionso the data loss is visible.🤖 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/worker.py` around lines 7519 - 7546, Update api_send_thread_message to persist thread_id as an indexed message column, then use that column in the legacy_records query to filter the requested thread. Add bounded, created_at-ordered pagination so the query does not load the entire participant history. In the per-row processing around decrypt_aes, replace the silent broad exception with capture_exception so decryption failures are observable.Source: Linters/SAST tools
2011-2018: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the
user_profileslookup so login cannot fail on it.
api_loginnow readsuser_profilesafter the password check succeeds. If that table is missing or the query fails, the exception propagates and the user receives a 500 instead of a token. The rest of the file already guards optional tables with_is_no_such_table_error(see lines 6041-6044 and 7254-7258).The avatar is decorative data. Treat a lookup failure as an empty avatar and let the login succeed.
🛡️ Proposed fix
- profile_row = await env.DB.prepare( - "SELECT avatar_url FROM user_profiles WHERE user_id=?" - ).bind(user_id).first() + profile_row = None + try: + profile_row = await env.DB.prepare( + "SELECT avatar_url FROM user_profiles WHERE user_id=?" + ).bind(user_id).first() + except Exception as exc: + if not _is_no_such_table_error(exc): + await capture_exception(exc, req, env, "api_login.profile_lookup") return ok(🤖 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/worker.py` around lines 2011 - 2018, Guard the user_profiles lookup in api_login with the existing _is_no_such_table_error handling pattern, and treat any lookup failure as an empty avatar while preserving successful avatar retrieval. Ensure exceptions from the optional table query do not propagate, allowing login to return the token and user data with avatar_url set to an empty string.
2855-2877: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep
enrollment_idout of the public certificate response.
/certificate.html?uuid=calls/api/certificates/{uuid}without anAuthorizationheader, so any holder of the certificate UUID can resolve the learner name, activity title, and internalenrollment_id. If certificate verification is public, removeenrollment_id; this internal enrollment key is not needed for display.🤖 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/worker.py` around lines 2855 - 2877, The api_get_certificate response must not expose the internal enrollment_id field. Remove enrollment_id from the object passed to ok while preserving the public certificate verification fields and existing lookup behavior.Sources: Coding guidelines, Linters/SAST tools
4050-4058: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd the missing
kickerkey for the/messagesSSR page.
render_ssr.record_pageaccessesconfig["kicker"]directly; when/messages.htmlis missing and the fallback SSR path runs, this raises aKeyError. Add a consistentkickervalue for the Messages page.🤖 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/worker.py` around lines 4050 - 4058, Add the missing “kicker” configuration key to the “/messages” entry in the SSR page configuration, using a consistent Messages-page value so render_ssr.record_page can access config["kicker"] during fallback rendering without raising a KeyError.
7306-7316: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winReplace the per-participant query with one batched query.
The loop runs one
SELECTagainstmessage_requestsfor every participant. An activity with 200 enrolled users produces 200 sequential round trips to D1 on a single request. The user list above already uses the batchedIN (...)pattern at lines 7295-7296, so the same approach fits here.Fetch every request row that involves the caller in one query, then build a lookup keyed by the other participant.
♻️ Proposed refactor
+ req_map = {} + if pids: + placeholders = ",".join(["?"] * len(pids)) + r_res = await env.DB.prepare( + "SELECT id, status, from_user_id, to_user_id FROM message_requests" + f" WHERE (from_user_id=? AND to_user_id IN ({placeholders}))" + f" OR (to_user_id=? AND from_user_id IN ({placeholders}))" + " ORDER BY created_at DESC" + ).bind(user["id"], *pids, user["id"], *pids).all() + for r in (r_res.results or []): + other = r.to_user_id if r.from_user_id == user["id"] else r.from_user_id + req_map.setdefault(other, r) + for pid in pids: - req_row = await env.DB.prepare( - "SELECT id, status FROM message_requests WHERE (from_user_id=? AND to_user_id=?) OR (from_user_id=? AND to_user_id=?) ORDER BY created_at DESC LIMIT 1" - ).bind(user["id"], pid, pid, user["id"]).first() - + req_row = req_map.get(pid) contacts.append({
setdefaultkeeps the most recent row, which matches theORDER BY created_at DESC LIMIT 1behaviour of the original query.🤖 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/worker.py` around lines 7306 - 7316, Replace the per-`pid` `env.DB.prepare` query inside the contacts loop with one batched `message_requests` query using the existing participant `IN (...)` pattern, filtering rows involving `user["id"]` and ordering newest first. Build a lookup keyed by the other participant, retaining the first row for each participant (for example via `setdefault`), then use that lookup to populate `existing_request_id_or_null` and `thread_status` while preserving the `"Participant"` and `"none"` defaults.
1334-1348: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd a uniqueness constraint for the
message_requestsuser pair.The table has no uniqueness constraint on
(from_user_id, to_user_id). Both writers rely on a SELECT-then-INSERT existence check instead:
api_send_message_requestat lines 7167-7175 checks for an existing row in either direction, then inserts.api_send_activity_message_requestat lines 7350-7360 does the same.Two concurrent requests can pass the check together and create two rows for the same pair of users.
api_list_message_threadsthen returns duplicate threads, and messages fetched byapi_get_thread_messagessplit across the duplicates.A plain
UNIQUE (from_user_id, to_user_id)does not stop the reversed pair, so a canonical pair key is the reliable fix. One option is a generated column plus a unique index:🛡️ Proposed fix: canonical pair uniqueness
"""CREATE TABLE IF NOT EXISTS message_requests ( id TEXT PRIMARY KEY, from_user_id TEXT NOT NULL, to_user_id TEXT NOT NULL, status TEXT NOT NULL DEFAULT 'pending', source TEXT NOT NULL DEFAULT 'email', activity_id TEXT, created_at TEXT NOT NULL DEFAULT (datetime('now')), FOREIGN KEY (from_user_id) REFERENCES users(id) ON DELETE CASCADE, FOREIGN KEY (to_user_id) REFERENCES users(id) ON DELETE CASCADE )""", + "CREATE UNIQUE INDEX IF NOT EXISTS idx_message_requests_pair ON message_requests(" + " MIN(from_user_id, to_user_id), MAX(from_user_id, to_user_id))", "CREATE INDEX IF NOT EXISTS idx_message_requests_to_user ON message_requests(to_user_id, status)",Keep the existing existence check as the fast path, and handle the constraint violation on insert as the fallback.
🤖 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/worker.py` around lines 1334 - 1348, Update the message_requests table definition to enforce uniqueness for each user pair regardless of direction, using a canonical pair key such as a generated column and unique index. Preserve the existing checks in api_send_message_request and api_send_activity_message_request as the fast path, and catch/handle unique-constraint failures during insertion as the concurrent-request fallback.
🤖 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.
Outside diff comments:
In `@src/worker.py`:
- Around line 7829-7837: The current global WaitingRoomDO design serializes all
room reads and writes through idFromName("global"), creating a potential
throughput bottleneck. Monitor concurrency and, if saturation occurs, refactor
the waiting-room Durable Object routing so create, join, leave, fulfill, and
delete use separate per-room Durable Objects while the global instance retains
only the listing index; keep the existing WAITING_ROOM_DO binding unchanged.
- Around line 7519-7546: Update api_send_thread_message to persist thread_id as
an indexed message column, then use that column in the legacy_records query to
filter the requested thread. Add bounded, created_at-ordered pagination so the
query does not load the entire participant history. In the per-row processing
around decrypt_aes, replace the silent broad exception with capture_exception so
decryption failures are observable.
- Around line 2011-2018: Guard the user_profiles lookup in api_login with the
existing _is_no_such_table_error handling pattern, and treat any lookup failure
as an empty avatar while preserving successful avatar retrieval. Ensure
exceptions from the optional table query do not propagate, allowing login to
return the token and user data with avatar_url set to an empty string.
- Around line 2855-2877: The api_get_certificate response must not expose the
internal enrollment_id field. Remove enrollment_id from the object passed to ok
while preserving the public certificate verification fields and existing lookup
behavior.
- Around line 4050-4058: Add the missing “kicker” configuration key to the
“/messages” entry in the SSR page configuration, using a consistent
Messages-page value so render_ssr.record_page can access config["kicker"] during
fallback rendering without raising a KeyError.
- Around line 7306-7316: Replace the per-`pid` `env.DB.prepare` query inside the
contacts loop with one batched `message_requests` query using the existing
participant `IN (...)` pattern, filtering rows involving `user["id"]` and
ordering newest first. Build a lookup keyed by the other participant, retaining
the first row for each participant (for example via `setdefault`), then use that
lookup to populate `existing_request_id_or_null` and `thread_status` while
preserving the `"Participant"` and `"none"` defaults.
- Around line 1334-1348: Update the message_requests table definition to enforce
uniqueness for each user pair regardless of direction, using a canonical pair
key such as a generated column and unique index. Preserve the existing checks in
api_send_message_request and api_send_activity_message_request as the fast path,
and catch/handle unique-constraint failures during insertion as the
concurrent-request fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 55b741df-6566-4c6d-976e-0ba16db2ec02
📒 Files selected for processing (2)
src/worker.pywrangler.toml
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
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.
This PR establishes the core discovery-to-course flow for Alpha One Labs by connecting the Waiting Room, Courses, and Teaching journeys end to end. It introduces dedicated pages for room detail, course listing, and teach-course creation, and adds backend support so creators can fulfill demand from waiting rooms into actual courses.
What’s Implemented
Waiting Room + Course Conversion Flow
Added waiting room detail experience and clickable room cards.
Added support for marking a waiting room as fulfilled with linked course metadata.
Exposed room-level fetch APIs for detail rendering and creator actions.
Courses Experience
Added dedicated Courses listing page and scripts.
Added Teach Course flow for instructors/hosts.
Integrated navigation updates so users can move smoothly across dashboard, waiting room, and courses.
Host Management Controls
Added host-side delete actions for:
Entire course/activity
Individual sessions within a course
Added corresponding backend endpoints with authorization checks and ownership validation.
Impact / Review Focus
Waiting room fulfill lifecycle and creator-only authorization.
Correctness of delete flows (activity/session) and data cleanup.
Demo
https://drive.google.com/file/d/1vQNKKGjwZYwT8LtRwKQEpbIl13ui2Djn/view?usp=sharing
Overview
This PR connects the Waiting Room, Courses, and Teaching journeys. Learners can create and join waiting rooms. Room creators can create courses from room data. Hosts can manage and delete courses and sessions.
Key Changes
WaitingRoomDOwith authenticated APIs for room listing, retrieval, creation, participation, deletion, and fulfillment.WAITING_ROOM_DOand its SQLite migration inwrangler.toml.Impact
Learners can request and discover content through waiting rooms. Creators can convert requests into courses with pre-filled data. Hosts can delete courses or sessions safely. Server-side authorization protects creator and host actions.