feat: Add waiting room DO and courses flows across learner and host journeys#49
Open
Lakshya-2440 wants to merge 1 commit intoalphaonelabs:mainfrom
Open
feat: Add waiting room DO and courses flows across learner and host journeys#49Lakshya-2440 wants to merge 1 commit intoalphaonelabs:mainfrom
Lakshya-2440 wants to merge 1 commit intoalphaonelabs:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 37 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
WalkthroughThis pull request introduces a complete "waiting rooms" feature for learning requests alongside a course management system. It adds nine new public-facing pages with client-side logic for browsing, creating, and managing waiting rooms and courses, plus corresponding backend support via a new Durable Object and API delete endpoints. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 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
| } | ||
| const token = readAuthValue('edu_token'); | ||
| const user = JSON.parse(readAuthValue('edu_user') || 'null'); | ||
| function esc(s) { return String(s).replace(/&/g,'&').replace(/</g,'<').replace(/>/g,'>'); } |
There was a problem hiding this comment.
esc() does not escape quotes — fine today, but it’s riding on UUID-only inputs.
esc only replaces & < >, yet at line 214 you inject session ids into a single-quoted HTML attribute:
'<button ... onclick="deleteSession(\'' + esc(s.id) + '\')">Delete Session</button>'Today s.id is a server-generated UUID so there’s no exploit, but any future change that lets user-controlled text flow through esc() into an attribute context becomes a stored-XSS foothold. Recommend hardening esc once (defense in depth) rather than relying on upstream invariants:
- function esc(s) { return String(s).replace(/&/g,'&').replace(/</g,'<').replace(/>/g,'>'); }
+ function esc(s) {
+ return String(s)
+ .replace(/&/g,'&').replace(/</g,'<').replace(/>/g,'>')
+ .replace(/"/g,'"').replace(/'/g,''');
+ }Bonus win: it then matches the escapeHtml used in courses.js / waitingroom.js so a future shared auth/escape module is a copy-paste.
As per coding guidelines: "Review HTML templates for ... XSS risks from unescaped user content."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/course.html` at 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.
Comment on lines
+144
to
+168
| async function deleteCourse() { | ||
| if (!token) { window.location.href = '/login.html'; return; } | ||
| if (!confirm('Delete this course and all its sessions? This cannot be undone.')) return; | ||
| const res = await fetch('/api/activities/delete', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json', Authorization: 'Bearer ' + token }, | ||
| body: JSON.stringify({ activity_id: actId }) | ||
| }); | ||
| const body = await res.json().catch(() => ({})); | ||
| if (!res.ok) throw new Error(body.error || 'Failed to delete course'); | ||
| window.location.href = '/courses.html'; | ||
| } | ||
|
|
||
| async function deleteSession(sessionId) { | ||
| if (!token) { window.location.href = '/login.html'; return; } | ||
| if (!confirm('Delete this session?')) return; | ||
| const res = await fetch('/api/sessions/delete', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json', Authorization: 'Bearer ' + token }, | ||
| body: JSON.stringify({ session_id: sessionId }) | ||
| }); | ||
| const body = await res.json().catch(() => ({})); | ||
| if (!res.ok) throw new Error(body.error || 'Failed to delete session'); | ||
| await loadActivity(); | ||
| } |
There was a problem hiding this comment.
Async onclick handlers swallow errors silently.
deleteCourse() and deleteSession() are async and throw new Error(...) on non-OK responses, but they’re invoked via onclick="deleteCourse()" so the returned promise rejection becomes an unhandled-rejection warning with no user feedback — the user just sees "nothing happened". Wrap the fetch in try/catch and surface the message (matching the pattern already used in joinActivity at lines 271-274):
async function deleteCourse() {
if (!token) { window.location.href = '/login.html'; return; }
if (!confirm('Delete this course and all its sessions? This cannot be undone.')) return;
- const res = await fetch('/api/activities/delete', { ... });
- const body = await res.json().catch(() => ({}));
- if (!res.ok) throw new Error(body.error || 'Failed to delete course');
- window.location.href = '/courses.html';
+ try {
+ const res = await fetch('/api/activities/delete', { ... });
+ const body = await res.json().catch(() => ({}));
+ if (!res.ok) throw new Error(body.error || 'Failed to delete course');
+ window.location.href = '/courses.html';
+ } catch (e) {
+ alert(e.message || 'Failed to delete course');
+ }
}Same for deleteSession.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function deleteCourse() { | |
| if (!token) { window.location.href = '/login.html'; return; } | |
| if (!confirm('Delete this course and all its sessions? This cannot be undone.')) return; | |
| const res = await fetch('/api/activities/delete', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json', Authorization: 'Bearer ' + token }, | |
| body: JSON.stringify({ activity_id: actId }) | |
| }); | |
| const body = await res.json().catch(() => ({})); | |
| if (!res.ok) throw new Error(body.error || 'Failed to delete course'); | |
| window.location.href = '/courses.html'; | |
| } | |
| async function deleteSession(sessionId) { | |
| if (!token) { window.location.href = '/login.html'; return; } | |
| if (!confirm('Delete this session?')) return; | |
| const res = await fetch('/api/sessions/delete', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json', Authorization: 'Bearer ' + token }, | |
| body: JSON.stringify({ session_id: sessionId }) | |
| }); | |
| const body = await res.json().catch(() => ({})); | |
| if (!res.ok) throw new Error(body.error || 'Failed to delete session'); | |
| await loadActivity(); | |
| } | |
| async function deleteCourse() { | |
| if (!token) { window.location.href = '/login.html'; return; } | |
| if (!confirm('Delete this course and all its sessions? This cannot be undone.')) return; | |
| try { | |
| const res = await fetch('/api/activities/delete', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json', Authorization: 'Bearer ' + token }, | |
| body: JSON.stringify({ activity_id: actId }) | |
| }); | |
| const body = await res.json().catch(() => ({})); | |
| if (!res.ok) throw new Error(body.error || 'Failed to delete course'); | |
| window.location.href = '/courses.html'; | |
| } catch (e) { | |
| alert(e.message || 'Failed to delete course'); | |
| } | |
| } | |
| async function deleteSession(sessionId) { | |
| if (!token) { window.location.href = '/login.html'; return; } | |
| if (!confirm('Delete this session?')) return; | |
| try { | |
| const res = await fetch('/api/sessions/delete', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json', Authorization: 'Bearer ' + token }, | |
| body: JSON.stringify({ session_id: sessionId }) | |
| }); | |
| const body = await res.json().catch(() => ({})); | |
| if (!res.ok) throw new Error(body.error || 'Failed to delete session'); | |
| await loadActivity(); | |
| } catch (e) { | |
| alert(e.message || 'Failed to delete session'); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/course.html` around lines 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.
Comment on lines
+50
to
+53
| <div class="nav-search"> | ||
| <svg width="13" height="13" fill="none" stroke="currentColor" stroke-width="2.2" viewBox="0 0 24 24"><circle cx="11" cy="11" r="8"/><path d="m21 21-4.35-4.35"/></svg> | ||
| <input type="text" placeholder="What do you want to learn?"/> | ||
| </div> |
There was a problem hiding this comment.
Nit: search <input> is missing an associated label.
HTMLHint flags this one. A placeholder alone isn’t accessible — screen readers need a programmatic label. Either wrap in a <label> or add aria-label:
- <input type="text" placeholder="What do you want to learn?"/>
+ <input type="text" aria-label="Search courses" placeholder="What do you want to learn?"/>As per coding guidelines: "Review HTML templates for accessibility (ARIA attributes, semantic elements)..."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div class="nav-search"> | |
| <svg width="13" height="13" fill="none" stroke="currentColor" stroke-width="2.2" viewBox="0 0 24 24"><circle cx="11" cy="11" r="8"/><path d="m21 21-4.35-4.35"/></svg> | |
| <input type="text" placeholder="What do you want to learn?"/> | |
| </div> | |
| <div class="nav-search"> | |
| <svg width="13" height="13" fill="none" stroke="currentColor" stroke-width="2.2" viewBox="0 0 24 24"><circle cx="11" cy="11" r="8"/><path d="m21 21-4.35-4.35"/></svg> | |
| <input type="text" aria-label="Search courses" placeholder="What do you want to learn?"/> | |
| </div> |
🧰 Tools
🪛 HTMLHint (1.9.2)
[warning] 52-52: No matching [ label ] tag found.
(input-requires-label)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/courses.html` around lines 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.
Comment on lines
+1
to
+22
| function readAuthValue(key) { | ||
| const fromSession = sessionStorage.getItem(key); | ||
| if (fromSession) return fromSession; | ||
| const fromLocal = localStorage.getItem(key); | ||
| if (fromLocal) { | ||
| sessionStorage.setItem(key, fromLocal); | ||
| localStorage.removeItem(key); | ||
| } | ||
| return fromLocal; | ||
| } | ||
|
|
||
| function clearAuth() { | ||
| sessionStorage.removeItem('edu_token'); | ||
| sessionStorage.removeItem('edu_user'); | ||
| localStorage.removeItem('edu_token'); | ||
| localStorage.removeItem('edu_user'); | ||
| } | ||
|
|
||
| function authHeaders() { | ||
| const token = readAuthValue('edu_token'); | ||
| return token ? { Authorization: `Bearer ${token}` } : {}; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Refactor opportunity: readAuthValue / clearAuth / authHeaders are duplicated across courses.js, waitingroom.js, waitingroom-detail.js, and inlined in course.html.
Four near-identical copies is a maintenance hazard — next time you rotate storage keys or tweak the session-over-local migration, you have to remember all four. Extracting into a small public/auth.js included before the page scripts would DRY this up nicely and keep auth behavior consistent. Purely a nice-to-have, but worth a follow-up before the surface grows further.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/courses.js` around lines 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.
Comment on lines
+81
to
+97
| function renderSessions(container, sessions, locked) { | ||
| if (!sessions.length) { | ||
| container.innerHTML = '<div class="session-sub">No sessions yet.</div>'; | ||
| return; | ||
| } | ||
| container.innerHTML = sessions.map((s) => { | ||
| const title = escapeHtml(s.title || 'Session'); | ||
| const time = [s.start_time, s.end_time].filter(Boolean).join(' – '); | ||
| const sub = locked ? 'Join to see location and details' : [s.location, s.description].filter(Boolean).join(' · '); | ||
| return ` | ||
| <div class="session"> | ||
| <p class="session-title">${title}</p> | ||
| <div class="session-sub">${escapeHtml(time || sub || '')}</div> | ||
| </div> | ||
| `; | ||
| }).join(''); | ||
| } |
There was a problem hiding this comment.
🐛 renderSessions drops sub whenever time is truthy — location/description never show.
On line 93 the template uses ${escapeHtml(time || sub || '')} so as soon as a session has any start_time/end_time, the || short-circuits and sub (location + description, or the "Join to see location and details" lock message) is never rendered. That defeats the whole point of computing sub a couple of lines above.
🔧 Proposed fix — render both lines
container.innerHTML = sessions.map((s) => {
const title = escapeHtml(s.title || 'Session');
const time = [s.start_time, s.end_time].filter(Boolean).join(' – ');
const sub = locked ? 'Join to see location and details' : [s.location, s.description].filter(Boolean).join(' · ');
return `
<div class="session">
<p class="session-title">${title}</p>
- <div class="session-sub">${escapeHtml(time || sub || '')}</div>
+ ${time ? `<div class="session-sub">${escapeHtml(time)}</div>` : ''}
+ ${sub ? `<div class="session-sub">${escapeHtml(sub)}</div>` : ''}
</div>
`;
}).join('');As per coding guidelines: "Review JavaScript/TypeScript for ... potential XSS vulnerabilities. Ensure DOM interactions are safe. Look for opportunities to improve readability and maintainability."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function renderSessions(container, sessions, locked) { | |
| if (!sessions.length) { | |
| container.innerHTML = '<div class="session-sub">No sessions yet.</div>'; | |
| return; | |
| } | |
| container.innerHTML = sessions.map((s) => { | |
| const title = escapeHtml(s.title || 'Session'); | |
| const time = [s.start_time, s.end_time].filter(Boolean).join(' – '); | |
| const sub = locked ? 'Join to see location and details' : [s.location, s.description].filter(Boolean).join(' · '); | |
| return ` | |
| <div class="session"> | |
| <p class="session-title">${title}</p> | |
| <div class="session-sub">${escapeHtml(time || sub || '')}</div> | |
| </div> | |
| `; | |
| }).join(''); | |
| } | |
| function renderSessions(container, sessions, locked) { | |
| if (!sessions.length) { | |
| container.innerHTML = '<div class="session-sub">No sessions yet.</div>'; | |
| return; | |
| } | |
| container.innerHTML = sessions.map((s) => { | |
| const title = escapeHtml(s.title || 'Session'); | |
| const time = [s.start_time, s.end_time].filter(Boolean).join(' – '); | |
| const sub = locked ? 'Join to see location and details' : [s.location, s.description].filter(Boolean).join(' · '); | |
| return ` | |
| <div class="session"> | |
| <p class="session-title">${title}</p> | |
| ${time ? `<div class="session-sub">${escapeHtml(time)}</div>` : ''} | |
| ${sub ? `<div class="session-sub">${escapeHtml(sub)}</div>` : ''} | |
| </div> | |
| `; | |
| }).join(''); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/courses.js` around lines 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.
Comment on lines
+330
to
+346
| // Lightweight real-time sync without websockets: poll every 3s. | ||
| let isRefreshing = false; | ||
| async function refreshRooms() { | ||
| if (isRefreshing) return; | ||
| isRefreshing = true; | ||
| try { | ||
| rooms = await fetchRooms(); | ||
| renderRooms(rooms, currentUser, currentParticipantId); | ||
| } finally { | ||
| isRefreshing = false; | ||
| } | ||
| } | ||
| const refreshInterval = setInterval(refreshRooms, 3000); | ||
| document.addEventListener('visibilitychange', () => { | ||
| if (!document.hidden) refreshRooms(); | ||
| }); | ||
| window.addEventListener('beforeunload', () => clearInterval(refreshInterval)); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
3-second polling is a fine stopgap, but a few small polish items.
- The
setIntervalis cleared onbeforeunload, which modern browsers don't reliably fire for tab close on mobile / BFCache. Usingpagehide(or both) is more robust. - When the tab becomes visible (line 343-345), consider debouncing against an in-flight refresh in addition to
isRefreshing— today a rapid visibility flicker still works, just noting. - Longer term: since you already have a Durable Object, this screams for a WebSocket push (the repo already has a precedent per the retrieved learning about
web_socket=on 101 responses in Python Workers). Happy to sketch it out as a follow-up.
Based on learnings: "For Cloudflare Python Workers, when constructing a 101 WebSocket upgrade response using Response imported from workers ..., use the snake_case keyword argument web_socket=<client>" — worth remembering if you go the WebSocket route.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/waitingroom.js` around lines 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.
Comment on lines
+43
to
+234
| import uuid | ||
| from workers import DurableObject, Response | ||
| class WaitingRoomDO(DurableObject): | ||
| def __init__(self, state, env): | ||
| super().__init__(state, env) | ||
| self.state = state | ||
| self.env = env | ||
| self.rooms = None # lazy load durable state | ||
|
|
||
| async def on_fetch(self, request): | ||
| url = urlparse(request.url) | ||
| method = request.method.upper() | ||
| path = url.path | ||
| auth_user = verify_token( | ||
| request.headers.get("Authorization") or "", | ||
| getattr(self.env, "JWT_SECRET", "") or "", | ||
| ) | ||
| if self.rooms is None: | ||
| stored_rooms = await self.state.storage.get("rooms") | ||
| if isinstance(stored_rooms, str) and stored_rooms: | ||
| self.rooms = json.loads(stored_rooms) | ||
| else: | ||
| self.rooms = {} | ||
|
|
||
| def _normalize_participants(raw): | ||
| items = raw if isinstance(raw, list) else [] | ||
| out = [] | ||
| for p in items: | ||
| if isinstance(p, dict): | ||
| pid = str(p.get("id") or "").strip() | ||
| name = str(p.get("name") or "").strip() or "anonymous" | ||
| if not pid: | ||
| pid = name.lower() | ||
| out.append({"id": pid, "name": name}) | ||
| elif isinstance(p, str): | ||
| name = p.strip() or "anonymous" | ||
| out.append({"id": name.lower(), "name": name}) | ||
| return out | ||
|
|
||
| m_room = re.fullmatch(r"/api/waitingroom/([A-Za-z0-9_-]+)", path) | ||
| if m_room and method == "GET": | ||
| room_id = m_room.group(1) | ||
| room = self.rooms.get(room_id) | ||
| if not room: | ||
| return Response( | ||
| json.dumps({"error": "Room not found"}), | ||
| status=404, | ||
| headers={"Content-Type": "application/json"}, | ||
| ) | ||
| return Response( | ||
| json.dumps({"room": room}), | ||
| headers={"Content-Type": "application/json"}, | ||
| ) | ||
|
|
||
| if path.endswith('/api/waitingrooms') and method == 'GET': | ||
| # List all rooms | ||
| return Response(json.dumps({"rooms": list(self.rooms.values())}), headers={"Content-Type": "application/json"}) | ||
| if path.endswith('/api/waitingroom/create') and method == 'POST': | ||
| data = await request.json() | ||
| room_id = 'r_' + str(uuid.uuid4()) | ||
| room = { | ||
| "id": room_id, | ||
| "title": data.get("title", ""), | ||
| "subject": data.get("subject", ""), | ||
| "desc": data.get("desc", ""), | ||
| "tags": data.get("tags", []), | ||
| "creator": auth_user["username"] if auth_user else data.get("creator", ""), | ||
| "creatorId": auth_user["id"] if auth_user else str(data.get("creatorId", "")).strip(), | ||
| "createdAt": data.get("createdAt", 0), | ||
| "fulfilled": False, | ||
| "fulfilledAt": 0, | ||
| "fulfilledBy": "", | ||
| "courseId": "", | ||
| "courseTitle": "", | ||
| "participants": [] | ||
| } | ||
| self.rooms[room_id] = room | ||
| await self.state.storage.put("rooms", json.dumps(self.rooms)) | ||
| return Response(json.dumps(room), headers={"Content-Type": "application/json"}) | ||
|
|
||
| if path.endswith('/api/waitingroom/join') and method == 'POST': | ||
| data = await request.json() | ||
| room_id = str(data.get("roomId") or data.get("room_id") or "").strip() | ||
| name = auth_user["username"] if auth_user else str(data.get("name") or data.get("user") or "").strip() | ||
| participant_id = auth_user["id"] if auth_user else str(data.get("participantId") or "").strip() | ||
| if not room_id: | ||
| return Response(json.dumps({"error": "roomId is required"}), status=400, headers={"Content-Type": "application/json"}) | ||
| room = self.rooms.get(room_id) | ||
| if not room: | ||
| return Response(json.dumps({"error": "Room not found"}), status=404, headers={"Content-Type": "application/json"}) | ||
| if not name: | ||
| name = "anonymous" | ||
| if not participant_id: | ||
| participant_id = name.lower() | ||
|
|
||
| participants = _normalize_participants(room.get("participants")) | ||
| updated = False | ||
| for p in participants: | ||
| if p["id"] == participant_id: | ||
| p["name"] = name | ||
| updated = True | ||
| break | ||
| if not updated: | ||
| participants.append({"id": participant_id, "name": name}) | ||
| room["participants"] = participants | ||
| self.rooms[room_id] = room | ||
| await self.state.storage.put("rooms", json.dumps(self.rooms)) | ||
| return Response(json.dumps(room), headers={"Content-Type": "application/json"}) | ||
|
|
||
| if path.endswith('/api/waitingroom/leave') and method == 'POST': | ||
| data = await request.json() | ||
| room_id = str(data.get("roomId") or data.get("room_id") or "").strip() | ||
| name = auth_user["username"] if auth_user else str(data.get("name") or data.get("user") or "").strip() | ||
| participant_id = auth_user["id"] if auth_user else str(data.get("participantId") or "").strip() | ||
| if not room_id or (not name and not participant_id): | ||
| return Response(json.dumps({"error": "roomId and participant identity are required"}), status=400, headers={"Content-Type": "application/json"}) | ||
| room = self.rooms.get(room_id) | ||
| if not room: | ||
| return Response(json.dumps({"error": "Room not found"}), status=404, headers={"Content-Type": "application/json"}) | ||
|
|
||
| participants = _normalize_participants(room.get("participants")) | ||
| if participant_id: | ||
| room["participants"] = [p for p in participants if p["id"] != participant_id] | ||
| else: | ||
| room["participants"] = [p for p in participants if p["name"].lower() != name.lower()] | ||
| self.rooms[room_id] = room | ||
| await self.state.storage.put("rooms", json.dumps(self.rooms)) | ||
| return Response(json.dumps(room), headers={"Content-Type": "application/json"}) | ||
|
|
||
| if path.endswith('/api/waitingroom/delete') and method == 'POST': | ||
| data = await request.json() | ||
| room_id = str(data.get("roomId") or data.get("room_id") or "").strip() | ||
| actor = auth_user["username"] if auth_user else str(data.get("actor") or data.get("name") or "").strip() | ||
| actor_id = auth_user["id"] if auth_user else str(data.get("actorId") or "").strip() | ||
| if not room_id: | ||
| return Response(json.dumps({"error": "roomId is required"}), status=400, headers={"Content-Type": "application/json"}) | ||
| room = self.rooms.get(room_id) | ||
| if not room: | ||
| return Response(json.dumps({"error": "Room not found"}), status=404, headers={"Content-Type": "application/json"}) | ||
| creator = str(room.get("creator") or "").strip() | ||
| creator_id = str(room.get("creatorId") or "").strip() | ||
| normalized_actor = actor.lower() | ||
| normalized_creator = creator.lower() | ||
| # Backward compatibility: | ||
| # some existing rooms may carry a stale/migrated creatorId while still having | ||
| # the correct creator name. Allow delete when either identity (id OR name) matches. | ||
| id_matches = bool(creator_id and actor_id and actor_id == creator_id) | ||
| name_matches = bool(actor and creator and normalized_actor == normalized_creator) | ||
| if creator_id and not (id_matches or name_matches): | ||
| return Response(json.dumps({"error": "Only the creator can delete this room"}), status=403, headers={"Content-Type": "application/json"}) | ||
| if not creator_id and actor and creator and not name_matches: | ||
| return Response(json.dumps({"error": "Only the creator can delete this room"}), status=403, headers={"Content-Type": "application/json"}) | ||
|
|
||
| del self.rooms[room_id] | ||
| await self.state.storage.put("rooms", json.dumps(self.rooms)) | ||
| return Response(json.dumps({"success": True, "roomId": room_id}), headers={"Content-Type": "application/json"}) | ||
|
|
||
| if path.endswith('/api/waitingroom/fulfill') and method == 'POST': | ||
| data = await request.json() | ||
| room_id = str(data.get("roomId") or data.get("room_id") or "").strip() | ||
| actor = auth_user["username"] if auth_user else str(data.get("actor") or data.get("name") or "").strip() | ||
| actor_id = auth_user["id"] if auth_user else str(data.get("actorId") or "").strip() | ||
| course_id = str(data.get("courseId") or "").strip() | ||
| course_title = str(data.get("courseTitle") or "").strip() | ||
| if not room_id: | ||
| return Response(json.dumps({"error": "roomId is required"}), status=400, headers={"Content-Type": "application/json"}) | ||
| if not course_id: | ||
| return Response(json.dumps({"error": "courseId is required"}), status=400, headers={"Content-Type": "application/json"}) | ||
| room = self.rooms.get(room_id) | ||
| if not room: | ||
| return Response(json.dumps({"error": "Room not found"}), status=404, headers={"Content-Type": "application/json"}) | ||
| creator = str(room.get("creator") or "").strip() | ||
| creator_id = str(room.get("creatorId") or "").strip() | ||
| normalized_actor = actor.lower() | ||
| normalized_creator = creator.lower() | ||
| id_matches = bool(creator_id and actor_id and actor_id == creator_id) | ||
| name_matches = bool(actor and creator and normalized_actor == normalized_creator) | ||
| if creator_id and not (id_matches or name_matches): | ||
| return Response(json.dumps({"error": "Only the creator can fulfill this room"}), status=403, headers={"Content-Type": "application/json"}) | ||
| if not creator_id and actor and creator and not name_matches: | ||
| return Response(json.dumps({"error": "Only the creator can fulfill this room"}), status=403, headers={"Content-Type": "application/json"}) | ||
|
|
||
| room["fulfilled"] = True | ||
| room["fulfilledAt"] = int(data.get("fulfilledAt") or 0) or int(js.Date.now()) | ||
| room["fulfilledBy"] = actor | ||
| room["courseId"] = course_id | ||
| room["courseTitle"] = course_title | ||
| self.rooms[room_id] = room | ||
| await self.state.storage.put("rooms", json.dumps(self.rooms)) | ||
| return Response(json.dumps(room), headers={"Content-Type": "application/json"}) | ||
|
|
||
| return Response(json.dumps({"error": "Not implemented"}), status=404, headers={"Content-Type": "application/json"}) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Is there a DO binding declared in wrangler config?
fd -t f -H 'wrangler\.(toml|jsonc?|yaml)$' --exec sh -c 'echo "=== $1 ==="; cat "$1"' _ {}
# 2) Is WaitingRoomDO referenced anywhere other than its definition?
rg -nP --type=py '\bWaitingRoomDO\b'
# 3) Is there any dispatcher branch for /api/waitingroom paths?
rg -nP -C2 '/api/waitingroom' --type=pyRepository: alphaonelabs/learn
Length of output: 3188
🏁 Script executed:
# Check the _dispatch function around lines 1604-1682
sed -n '1604,1682p' src/worker.py
# Check the import section around lines 236-238
sed -n '230,245p' src/worker.py
# Verify durable object bindings in wrangler config
rg -A 20 'durable_objects|DurableObject' wrangler.tomlRepository: alphaonelabs/learn
Length of output: 3718
🏁 Script executed:
# Extract the __init__ and _normalize_participants method signatures
sed -n '45,80p' src/worker.py | head -20Repository: alphaonelabs/learn
Length of output: 799
🚨 WaitingRoomDO is unreachable — missing binding and dispatcher routing.
Good news: the waiting room feature handlers are well-structured internally. However, they won't execute in production because:
-
No durable object binding —
wrangler.tomldefines onlyASSETSandDBbindings. Add a DO binding (e.g.,WAITING_ROOM = { class_name = "WaitingRoomDO" }) and configure it properly. -
No dispatcher routing — The
_dispatchfunction (lines 1604+) handles/api/activities,/api/sessions,/api/tags, etc., but has no branch for/api/waitingroom*paths. Any request to create/join/leave/delete/fulfill a room currently returnserr("API endpoint not found", 404). Add a dispatcher branch that forwards waitingroom requests to the DO stub.
Additionally, please address two PEP 8 issues:
- Import placement — The class is defined at lines 45–234, but
urlparse(line 53) andjs(line 226) are imported at lines 236–238. Move all imports to the top of the file per PEP 8 Section E402. - Missing type annotations —
__init__is missing a return type (-> None), and_normalize_participantsis missing parameter and return type hints. Add these to satisfy ruff rules ANN204 and ANN202.
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 46-46: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
[warning] 52-52: Too many return statements (22 > 6)
(PLR0911)
[warning] 52-52: Too many branches (31 > 12)
(PLR0912)
[warning] 52-52: Too many statements (116 > 50)
(PLR0915)
[warning] 67-67: Missing return type annotation for private function _normalize_participants
(ANN202)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/worker.py` around lines 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.
Comment on lines
+82
to
+99
| m_room = re.fullmatch(r"/api/waitingroom/([A-Za-z0-9_-]+)", path) | ||
| if m_room and method == "GET": | ||
| room_id = m_room.group(1) | ||
| room = self.rooms.get(room_id) | ||
| if not room: | ||
| return Response( | ||
| json.dumps({"error": "Room not found"}), | ||
| status=404, | ||
| headers={"Content-Type": "application/json"}, | ||
| ) | ||
| return Response( | ||
| json.dumps({"room": room}), | ||
| headers={"Content-Type": "application/json"}, | ||
| ) | ||
|
|
||
| if path.endswith('/api/waitingrooms') and method == 'GET': | ||
| # List all rooms | ||
| return Response(json.dumps({"rooms": list(self.rooms.values())}), headers={"Content-Type": "application/json"}) |
There was a problem hiding this comment.
Path matching is a bit loose — tighten to exact equality.
Two small things worth hardening here:
- Regex collides with reserved sub-paths.
r"/api/waitingroom/([A-Za-z0-9_-]+)"also matches/api/waitingroom/create|join|leave|delete|fulfill. Today you’re saved only because those arePOSTand the regex block is gated onGET, but a future reviewer could easily break this. Exclude the reserved words (or check the POST endpoints first):m_room = re.fullmatch(r"/api/waitingroom/(?!create|join|leave|delete|fulfill\b)([A-Za-z0-9_-]+)", path)
path.endswith(...)(lines 97, 100, 123, 152, 172, 200) will also match any path that happens to end in that suffix. Preferpath == '/api/waitingrooms'etc. — it’s both faster and unambiguous.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/worker.py` around lines 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.
Comment on lines
+186
to
+198
| # Backward compatibility: | ||
| # some existing rooms may carry a stale/migrated creatorId while still having | ||
| # the correct creator name. Allow delete when either identity (id OR name) matches. | ||
| id_matches = bool(creator_id and actor_id and actor_id == creator_id) | ||
| name_matches = bool(actor and creator and normalized_actor == normalized_creator) | ||
| if creator_id and not (id_matches or name_matches): | ||
| return Response(json.dumps({"error": "Only the creator can delete this room"}), status=403, headers={"Content-Type": "application/json"}) | ||
| if not creator_id and actor and creator and not name_matches: | ||
| return Response(json.dumps({"error": "Only the creator can delete this room"}), status=403, headers={"Content-Type": "application/json"}) | ||
|
|
||
| del self.rooms[room_id] | ||
| await self.state.storage.put("rooms", json.dumps(self.rooms)) | ||
| return Response(json.dumps({"success": True, "roomId": room_id}), headers={"Content-Type": "application/json"}) |
There was a problem hiding this comment.
🔐 Authorization bypass when a room has no creatorId and the caller provides no actor.
The two-branch permission check has a gap:
if creator_id and not (id_matches or name_matches): # branch A
return 403
if not creator_id and actor and creator and not name_matches: # branch B
return 403
If creator_id is empty AND either actor or creator is empty, neither branch fires and the delete proceeds. For an unauthenticated request (auth_user is None) with actor omitted from the JSON body, actor becomes "" — so any anonymous caller can delete any legacy room that lacks a creatorId. The exact same pattern exists in fulfill at lines 220-223 (an anonymous caller could mark any no-creatorId room as fulfilled and attach arbitrary course metadata).
Suggested tightening — require auth, and never grant access on empty identity:
🛡️ Proposed fix
- actor = auth_user["username"] if auth_user else str(data.get("actor") or data.get("name") or "").strip()
- actor_id = auth_user["id"] if auth_user else str(data.get("actorId") or "").strip()
+ if not auth_user:
+ return Response(json.dumps({"error": "Authentication required"}), status=401, headers={"Content-Type": "application/json"})
+ actor = str(auth_user.get("username") or "").strip()
+ actor_id = str(auth_user.get("id") or "").strip()
...
- id_matches = bool(creator_id and actor_id and actor_id == creator_id)
- name_matches = bool(actor and creator and normalized_actor == normalized_creator)
- if creator_id and not (id_matches or name_matches):
- return Response(json.dumps({"error": "Only the creator can delete this room"}), status=403, headers={"Content-Type": "application/json"})
- if not creator_id and actor and creator and not name_matches:
- return Response(json.dumps({"error": "Only the creator can delete this room"}), status=403, headers={"Content-Type": "application/json"})
+ id_matches = bool(creator_id and actor_id and actor_id == creator_id)
+ name_matches = bool(actor and creator and normalized_actor == normalized_creator)
+ if not (id_matches or name_matches):
+ return Response(json.dumps({"error": "Only the creator can delete this room"}), status=403, headers={"Content-Type": "application/json"})Please apply the same treatment to the fulfill handler.
As per coding guidelines: "Check for security vulnerabilities ... Ensure proper error handling."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/worker.py` around lines 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.
Comment on lines
+1409
to
+1427
| try: | ||
| await env.DB.prepare( | ||
| "DELETE FROM session_attendance WHERE session_id IN (SELECT id FROM sessions WHERE activity_id=?)" | ||
| ).bind(act_id).run() | ||
| await env.DB.prepare( | ||
| "DELETE FROM sessions WHERE activity_id=?" | ||
| ).bind(act_id).run() | ||
| await env.DB.prepare( | ||
| "DELETE FROM enrollments WHERE activity_id=?" | ||
| ).bind(act_id).run() | ||
| await env.DB.prepare( | ||
| "DELETE FROM activity_tags WHERE activity_id=?" | ||
| ).bind(act_id).run() | ||
| await env.DB.prepare( | ||
| "DELETE FROM activities WHERE id=? AND host_id=?" | ||
| ).bind(act_id, user["id"]).run() | ||
| except Exception as e: | ||
| await capture_exception(e, req, env, "api_delete_activity.delete") | ||
| return err("Failed to delete activity — please try again", 500) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Cloudflare D1's Python Workers API support env.DB.batch([...]) for atomic multi-statement execution?
💡 Result:
Yes, Cloudflare D1's Python Workers API supports env.DB.batch([...]) for atomic multi-statement execution. Official documentation provides Python examples using await self.env.DB.batch(to_js([...])) where the array contains bound prepared statements. Batched statements execute sequentially as SQL transactions: if any statement fails, the entire batch aborts or rolls back, ensuring atomicity. Usage requires from pyodide.ffi import to_js to convert the Python list to JavaScript Array for the D1 binding.
Citations:
- 1: https://developers.cloudflare.com/d1/worker-api/d1-database/index.md
- 2: https://developers.cloudflare.com:2087/d1/worker-api/d1-database/
- 3: https://developers.cloudflare.com/d1/worker-api/d1-database/
- 4: https://developers.cloudflare.com/d1/worker-api/
🏁 Script executed:
# First, check if the file exists and get its line count
wc -l src/worker.pyRepository: alphaonelabs/learn
Length of output: 80
🏁 Script executed:
# Read the specific section mentioned in the review (lines 1409-1427) with some context
sed -n '1400,1440p' src/worker.pyRepository: alphaonelabs/learn
Length of output: 1497
🏁 Script executed:
# Search for env.DB.batch usage patterns in the file to see if batching is already used elsewhere
rg "env\.DB\.batch" src/worker.py -A 3Repository: alphaonelabs/learn
Length of output: 44
🏁 Script executed:
# Search for waiting room invalidation or similar cleanup after activity deletion
rg -i "waiting.room|fulfilled|courseId" src/worker.py -B 2 -A 2Repository: alphaonelabs/learn
Length of output: 1627
🏁 Script executed:
# Search for any cleanup or invalidation of waiting rooms in the context of activity deletion
rg "api_delete_activity|delete.*activity" src/worker.py -B 5 -A 15 | head -100Repository: alphaonelabs/learn
Length of output: 2307
🏁 Script executed:
# Search for tests related to delete_activity
fd -name "*.py" -type f | xargs rg "delete_activity|test.*delete" | head -50Repository: alphaonelabs/learn
Length of output: 526
🏁 Script executed:
# Search for where courseId is cleared or invalidated in waiting rooms
rg "courseId.*=" src/worker.py -B 3 -A 3 | head -80Repository: alphaonelabs/learn
Length of output: 961
🏁 Script executed:
# Check if there's any Durable Object sync or cleanup mechanism for waiting rooms
rg -i "durable.object|waiting.room|state.*clear|invalidate" src/worker.py -A 2 | head -100Repository: alphaonelabs/learn
Length of output: 44
🏁 Script executed:
# Look for how waiting rooms and activities are linked - search for references between them
rg "activity.*id|courseId" src/worker.py | grep -i "waiting\|room\|course" | head -30Repository: alphaonelabs/learn
Length of output: 335
🏁 Script executed:
# Check if there's a way to sync or invalidate waiting rooms from the main worker
rg -i "state.storage|rooms\[" src/worker.py -B 2 -A 2 | head -100Repository: alphaonelabs/learn
Length of output: 1892
🏁 Script executed:
# Search for any logic that checks if a course/activity exists before rendering it
rg -i "404|not.*found|course.*title" src/worker.py | head -30Repository: alphaonelabs/learn
Length of output: 1411
🏁 Script executed:
# Look for the Durable Object class definition to understand the structure
rg "class.*Durable|self.rooms|self.state" src/worker.py | head -50Repository: alphaonelabs/learn
Length of output: 1260
🏁 Script executed:
# Check if there's any documentation or comments about expected exceptions from D1
rg "D1|prepare|run\(\)" src/worker.py | grep -i "except\|error\|exception" | head -20Repository: alphaonelabs/learn
Length of output: 150
🏁 Script executed:
# Look for other error handling patterns in the file to see best practices used
rg "except.*Error|except.*Exception" src/worker.py -B 2 -A 3 | head -80Repository: alphaonelabs/learn
Length of output: 2567
🏁 Script executed:
# Check if there's a frontend file that renders waiting room course links
fd -type f \( -name "*.js" -o -name "*.html" -o -name "*.jsx" -o -name "*.ts" \) | xargs rg -l "waiting.*room\|courseId" 2>/dev/null | head -10Repository: alphaonelabs/learn
Length of output: 232
🏁 Script executed:
# Understand the relationship: is courseId in waiting rooms expected to match an activity_id?
rg "fulfilled.*course|courseId" src/worker.py -B 5 -A 5 | head -100Repository: alphaonelabs/learn
Length of output: 2377
🏁 Script executed:
# Check if there's a way to signal or invalidate from the main worker to the Durable Object
rg "WaitingRoomDO|fetch.*waiting|fetch.*durable" src/worker.py | head -20Repository: alphaonelabs/learn
Length of output: 97
🏁 Script executed:
# See if there's any cleanup logic or hooks for when an activity is deleted
rg -i "delete.*hook|cleanup|invalidate|sync" src/worker.py | head -20Repository: alphaonelabs/learn
Length of output: 1064
🏁 Script executed:
# Confirm the import/usage of to_js for batch conversion
rg "from pyodide|to_js|import.*batch" src/worker.py | head -10Repository: alphaonelabs/learn
Length of output: 686
Use env.DB.batch() for atomic cascade deletion and clean up stale waiting room references.
The cascade delete is not atomic — each await env.DB.prepare(...).run() is its own D1 transaction, so a mid-flight failure (e.g., activity_tags delete fails after enrollments succeeds) leaves the database inconsistent with orphaned rows. More critically, when an activity is deleted, the waiting room's courseId and courseTitle fields remain in the Durable Object storage — if a user visits that waiting room, they'll see a link to a course that no longer exists.
Two actionable fixes:
-
Batch the statements for atomicity:
stmts = [ env.DB.prepare("DELETE FROM session_attendance WHERE session_id IN (SELECT id FROM sessions WHERE activity_id=?)").bind(act_id), env.DB.prepare("DELETE FROM sessions WHERE activity_id=?").bind(act_id), env.DB.prepare("DELETE FROM enrollments WHERE activity_id=?").bind(act_id), env.DB.prepare("DELETE FROM activity_tags WHERE activity_id=?").bind(act_id), env.DB.prepare("DELETE FROM activities WHERE id=? AND host_id=?").bind(act_id, user["id"]), ] await env.DB.batch(to_js(stmts))
(
to_jsis already imported and D1's Python API supports atomic batch execution.) -
Invalidate stale waiting room links by either (a) exposing a separate API endpoint for the waiting room Durable Object to clear fulfilled room fields when an activity is deleted, or (b) adding a validity check in the frontend before rendering the "Linked Course" section so it gracefully handles missing courses.
The bare except Exception at line 1425 is consistent with your error-logging pattern throughout the codebase (e.g., decrypt_aes, parse_json_object), where capture_exception() is the sole sink — this is fine.
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 1425-1425: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/worker.py` around lines 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.
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
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.
Comment on lines
+1384
to
+1423
| async def api_delete_activity(req, env): | ||
| user = verify_token(req.headers.get("Authorization"), env.JWT_SECRET) | ||
| if not user: | ||
| return err("Authentication required", 401) | ||
|
|
||
| body, bad_resp = await parse_json_object(req) | ||
| if bad_resp: | ||
| return bad_resp | ||
|
|
||
| act_id = (body.get("activity_id") or "").strip() | ||
| if not act_id: | ||
| return err("activity_id is required") | ||
|
|
||
| owned = await env.DB.prepare( | ||
| "SELECT id FROM activities WHERE id=? AND host_id=?" | ||
| ).bind(act_id, user["id"]).first() | ||
| if not owned: | ||
| return err("Activity not found or access denied", 404) | ||
|
|
||
| try: | ||
| await env.DB.prepare( | ||
| "DELETE FROM session_attendance WHERE session_id IN (SELECT id FROM sessions WHERE activity_id=?)" | ||
| ).bind(act_id).run() | ||
| await env.DB.prepare( | ||
| "DELETE FROM sessions WHERE activity_id=?" | ||
| ).bind(act_id).run() | ||
| await env.DB.prepare( | ||
| "DELETE FROM enrollments WHERE activity_id=?" | ||
| ).bind(act_id).run() | ||
| await env.DB.prepare( | ||
| "DELETE FROM activity_tags WHERE activity_id=?" | ||
| ).bind(act_id).run() | ||
| await env.DB.prepare( | ||
| "DELETE FROM activities WHERE id=? AND host_id=?" | ||
| ).bind(act_id, user["id"]).run() | ||
| except Exception as e: | ||
| await capture_exception(e, req, env, "api_delete_activity.delete") | ||
| return err("Failed to delete activity — please try again", 500) | ||
|
|
||
| return ok(None, "Activity deleted") |
There was a problem hiding this comment.
New destructive endpoint /api/activities/delete is introduced here, but there are no tests covering authorization (non-host/other host), required fields, and cascade deletion behavior. The repo has comprehensive pytest coverage for other API handlers; please add tests similar to tests/test_api_activities.py and tests/test_dispatcher.py to cover this handler.
Comment on lines
+1426
to
+1458
| async def api_delete_session(req, env): | ||
| user = verify_token(req.headers.get("Authorization"), env.JWT_SECRET) | ||
| if not user: | ||
| return err("Authentication required", 401) | ||
|
|
||
| body, bad_resp = await parse_json_object(req) | ||
| if bad_resp: | ||
| return bad_resp | ||
|
|
||
| session_id = (body.get("session_id") or "").strip() | ||
| if not session_id: | ||
| return err("session_id is required") | ||
|
|
||
| session = await env.DB.prepare( | ||
| "SELECT s.id,s.activity_id FROM sessions s" | ||
| " JOIN activities a ON a.id=s.activity_id" | ||
| " WHERE s.id=? AND a.host_id=?" | ||
| ).bind(session_id, user["id"]).first() | ||
| if not session: | ||
| return err("Session not found or access denied", 404) | ||
|
|
||
| try: | ||
| await env.DB.prepare( | ||
| "DELETE FROM session_attendance WHERE session_id=?" | ||
| ).bind(session_id).run() | ||
| await env.DB.prepare( | ||
| "DELETE FROM sessions WHERE id=?" | ||
| ).bind(session_id).run() | ||
| except Exception as e: | ||
| await capture_exception(e, req, env, "api_delete_session.delete") | ||
| return err("Failed to delete session — please try again", 500) | ||
|
|
||
| return ok(None, "Session deleted") |
There was a problem hiding this comment.
New destructive endpoint /api/sessions/delete is introduced here, but there are no tests covering authorization (non-host/other host), required fields, and deletion effects (session_attendance + sessions). Given existing pytest coverage for sessions creation/tags, add tests for this handler as well.
|
|
||
| <div class="section-heading"> | ||
| <h2>Open Waiting Rooms</h2> | ||
| <span class="count-badge" id="room-count">8</span> |
There was a problem hiding this comment.
The initial room count is hard-coded to 8; this will be incorrect until the first fetch completes (and will remain wrong if the fetch fails). Initialize this to 0 or an empty/loading state and let waitingroom.js set the real count.
Suggested change
| <span class="count-badge" id="room-count">8</span> | |
| <span class="count-badge" id="room-count">0</span> |
Comment on lines
+116
to
+117
| <label for="image">Course image</label> | ||
| <input id="image" class="file-input" type="file" accept="image/*" /> |
There was a problem hiding this comment.
This form includes a "Course image" file input, but teach-course.js never reads/uploads it, so users will think the image is being used when it is silently ignored. Either implement upload handling (and backend support) or remove/disable this field until it's supported.
Suggested change
| <label for="image">Course image</label> | |
| <input id="image" class="file-input" type="file" accept="image/*" /> | |
| <label for="image">Course image (not yet supported)</label> | |
| <input id="image" class="file-input" type="file" accept="image/*" disabled aria-disabled="true" /> | |
| <div class="hint">Image upload is not available yet. Please create your course without an image for now.</div> |
Comment on lines
+100
to
+102
| if path.endswith('/api/waitingroom/create') and method == 'POST': | ||
| data = await request.json() | ||
| room_id = 'r_' + str(uuid.uuid4()) |
There was a problem hiding this comment.
The WaitingRoomDO handlers use await request.json() directly; invalid/malformed JSON will raise and be caught only by the top-level on_fetch(), returning a generic 500. The rest of this codebase uses parse_json_object() to return a 400 with a clear message—consider using the same pattern here (or wrapping JSON parsing in try/except) so clients get a consistent 400 on bad input.
Comment on lines
+147
to
+167
| const res = await fetch('/api/activities/delete', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json', Authorization: 'Bearer ' + token }, | ||
| body: JSON.stringify({ activity_id: actId }) | ||
| }); | ||
| const body = await res.json().catch(() => ({})); | ||
| if (!res.ok) throw new Error(body.error || 'Failed to delete course'); | ||
| window.location.href = '/courses.html'; | ||
| } | ||
|
|
||
| async function deleteSession(sessionId) { | ||
| if (!token) { window.location.href = '/login.html'; return; } | ||
| if (!confirm('Delete this session?')) return; | ||
| const res = await fetch('/api/sessions/delete', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json', Authorization: 'Bearer ' + token }, | ||
| body: JSON.stringify({ session_id: sessionId }) | ||
| }); | ||
| const body = await res.json().catch(() => ({})); | ||
| if (!res.ok) throw new Error(body.error || 'Failed to delete session'); | ||
| await loadActivity(); |
There was a problem hiding this comment.
deleteSession() has the same unhandled-error problem as deleteCourse(): it throws on failure but is called from an inline onclick, so the user gets no feedback if the API call fails. Wrap the body in try/catch and surface the error (alert/toast) rather than throwing.
Suggested change
| const res = await fetch('/api/activities/delete', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json', Authorization: 'Bearer ' + token }, | |
| body: JSON.stringify({ activity_id: actId }) | |
| }); | |
| const body = await res.json().catch(() => ({})); | |
| if (!res.ok) throw new Error(body.error || 'Failed to delete course'); | |
| window.location.href = '/courses.html'; | |
| } | |
| async function deleteSession(sessionId) { | |
| if (!token) { window.location.href = '/login.html'; return; } | |
| if (!confirm('Delete this session?')) return; | |
| const res = await fetch('/api/sessions/delete', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json', Authorization: 'Bearer ' + token }, | |
| body: JSON.stringify({ session_id: sessionId }) | |
| }); | |
| const body = await res.json().catch(() => ({})); | |
| if (!res.ok) throw new Error(body.error || 'Failed to delete session'); | |
| await loadActivity(); | |
| try { | |
| const res = await fetch('/api/activities/delete', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json', Authorization: 'Bearer ' + token }, | |
| body: JSON.stringify({ activity_id: actId }) | |
| }); | |
| const body = await res.json().catch(() => ({})); | |
| if (!res.ok) throw new Error(body.error || 'Failed to delete course'); | |
| window.location.href = '/courses.html'; | |
| } catch (e) { | |
| alert(e.message || 'Failed to delete course'); | |
| } | |
| } | |
| async function deleteSession(sessionId) { | |
| if (!token) { window.location.href = '/login.html'; return; } | |
| if (!confirm('Delete this session?')) return; | |
| try { | |
| const res = await fetch('/api/sessions/delete', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json', Authorization: 'Bearer ' + token }, | |
| body: JSON.stringify({ session_id: sessionId }) | |
| }); | |
| const body = await res.json().catch(() => ({})); | |
| if (!res.ok) throw new Error(body.error || 'Failed to delete session'); | |
| await loadActivity(); | |
| } catch (e) { | |
| alert(e.message || 'Failed to delete session'); | |
| } |
Comment on lines
+45
to
+52
| class WaitingRoomDO(DurableObject): | ||
| def __init__(self, state, env): | ||
| super().__init__(state, env) | ||
| self.state = state | ||
| self.env = env | ||
| self.rooms = None # lazy load durable state | ||
|
|
||
| async def on_fetch(self, request): |
There was a problem hiding this comment.
WaitingRoomDO is defined here, but there is no routing in _dispatch() to forward /api/waitingroom* requests to a Durable Object stub (and wrangler.toml currently only binds CLASSROOM_DO). As-is, these endpoints will return "API endpoint not found". Add a Durable Object binding + migration for WaitingRoomDO and a dispatch branch (similar to the ClassroomDO block) that routes /api/waitingrooms and /api/waitingroom/* to a consistent DO instance.
Comment on lines
+87
to
+95
| return Response( | ||
| json.dumps({"error": "Room not found"}), | ||
| status=404, | ||
| headers={"Content-Type": "application/json"}, | ||
| ) | ||
| return Response( | ||
| json.dumps({"room": room}), | ||
| headers={"Content-Type": "application/json"}, | ||
| ) |
There was a problem hiding this comment.
WaitingRoomDO responses don't include the standard CORS headers used by the rest of the API (see json_resp() / _CORS in this file). If these endpoints are called from a different origin (or if you want consistent API behavior), include the same CORS headers in these Response objects (or reuse json_resp/ok/err helpers).
Comment on lines
+175
to
+195
| actor = auth_user["username"] if auth_user else str(data.get("actor") or data.get("name") or "").strip() | ||
| actor_id = auth_user["id"] if auth_user else str(data.get("actorId") or "").strip() | ||
| if not room_id: | ||
| return Response(json.dumps({"error": "roomId is required"}), status=400, headers={"Content-Type": "application/json"}) | ||
| room = self.rooms.get(room_id) | ||
| if not room: | ||
| return Response(json.dumps({"error": "Room not found"}), status=404, headers={"Content-Type": "application/json"}) | ||
| creator = str(room.get("creator") or "").strip() | ||
| creator_id = str(room.get("creatorId") or "").strip() | ||
| normalized_actor = actor.lower() | ||
| normalized_creator = creator.lower() | ||
| # Backward compatibility: | ||
| # some existing rooms may carry a stale/migrated creatorId while still having | ||
| # the correct creator name. Allow delete when either identity (id OR name) matches. | ||
| id_matches = bool(creator_id and actor_id and actor_id == creator_id) | ||
| name_matches = bool(actor and creator and normalized_actor == normalized_creator) | ||
| if creator_id and not (id_matches or name_matches): | ||
| return Response(json.dumps({"error": "Only the creator can delete this room"}), status=403, headers={"Content-Type": "application/json"}) | ||
| if not creator_id and actor and creator and not name_matches: | ||
| return Response(json.dumps({"error": "Only the creator can delete this room"}), status=403, headers={"Content-Type": "application/json"}) | ||
|
|
There was a problem hiding this comment.
/api/waitingroom/delete allows the caller to supply actor/actorId from the request body when not authenticated. For rooms without a creatorId, this effectively allows spoofing the creator name and deleting the room without a valid token. Require authentication for delete, and derive actor identity exclusively from the verified token (keep backward-compat by migrating/repairing creatorId rather than trusting the body).
Suggested change
| actor = auth_user["username"] if auth_user else str(data.get("actor") or data.get("name") or "").strip() | |
| actor_id = auth_user["id"] if auth_user else str(data.get("actorId") or "").strip() | |
| if not room_id: | |
| return Response(json.dumps({"error": "roomId is required"}), status=400, headers={"Content-Type": "application/json"}) | |
| room = self.rooms.get(room_id) | |
| if not room: | |
| return Response(json.dumps({"error": "Room not found"}), status=404, headers={"Content-Type": "application/json"}) | |
| creator = str(room.get("creator") or "").strip() | |
| creator_id = str(room.get("creatorId") or "").strip() | |
| normalized_actor = actor.lower() | |
| normalized_creator = creator.lower() | |
| # Backward compatibility: | |
| # some existing rooms may carry a stale/migrated creatorId while still having | |
| # the correct creator name. Allow delete when either identity (id OR name) matches. | |
| id_matches = bool(creator_id and actor_id and actor_id == creator_id) | |
| name_matches = bool(actor and creator and normalized_actor == normalized_creator) | |
| if creator_id and not (id_matches or name_matches): | |
| return Response(json.dumps({"error": "Only the creator can delete this room"}), status=403, headers={"Content-Type": "application/json"}) | |
| if not creator_id and actor and creator and not name_matches: | |
| return Response(json.dumps({"error": "Only the creator can delete this room"}), status=403, headers={"Content-Type": "application/json"}) | |
| if not room_id: | |
| return Response(json.dumps({"error": "roomId is required"}), status=400, headers={"Content-Type": "application/json"}) | |
| if not auth_user: | |
| return Response(json.dumps({"error": "Authentication is required"}), status=401, headers={"Content-Type": "application/json"}) | |
| actor = str(auth_user.get("username") or "").strip() | |
| actor_id = str(auth_user.get("id") or "").strip() | |
| room = self.rooms.get(room_id) | |
| if not room: | |
| return Response(json.dumps({"error": "Room not found"}), status=404, headers={"Content-Type": "application/json"}) | |
| creator = str(room.get("creator") or "").strip() | |
| creator_id = str(room.get("creatorId") or "").strip() | |
| normalized_actor = actor.lower() | |
| normalized_creator = creator.lower() | |
| if creator_id: | |
| if not actor_id or actor_id != creator_id: | |
| return Response(json.dumps({"error": "Only the creator can delete this room"}), status=403, headers={"Content-Type": "application/json"}) | |
| else: | |
| # Backward compatibility for legacy rooms created without creatorId: | |
| # only allow repair when the creator name matches the authenticated user, | |
| # and derive the repaired creatorId exclusively from the verified token. | |
| if not actor or not creator or normalized_actor != normalized_creator: | |
| return Response(json.dumps({"error": "Only the creator can delete this room"}), status=403, headers={"Content-Type": "application/json"}) | |
| if actor_id: | |
| room["creatorId"] = actor_id |
Comment on lines
+225
to
+230
| room["fulfilled"] = True | ||
| room["fulfilledAt"] = int(data.get("fulfilledAt") or 0) or int(js.Date.now()) | ||
| room["fulfilledBy"] = actor | ||
| room["courseId"] = course_id | ||
| room["courseTitle"] = course_title | ||
| self.rooms[room_id] = room |
There was a problem hiding this comment.
Fulfillment currently overwrites room state unconditionally. If a room is already fulfilled, this endpoint can be called again to change courseId/courseTitle (and fulfilledBy/fulfilledAt). Consider rejecting when room["fulfilled"] is already true (409), or making the operation idempotent by returning the existing fulfilled room without mutating it.
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 end-to-end by introducing new UI pages, client-side scripts, and backend endpoints that enable learners to request learning content and instructors to create courses from demand signals.
Key Additions
Frontend – Waiting Room Experience
waitingroom.html,waitingroom.js): New landing page displaying open/fulfilled rooms with join/leave participation controls, room creation modal, polling for real-time updates, and toast notifications for user actions.waitingroom-detail.html,waitingroom-detail.js): New detail view showing room metadata (title, description, creator, participants, tags), participant list, join/leave toggles, and creator-only actions to teach a course or delete the room.Frontend – Courses & Teaching
courses.html,courses.js): New page displaying the authenticated user's courses with expandable session details, join CTAs for non-enrolled users, and navigation to course creation.teach-course.html,teach-course.js): New form-based UI for instructors to create courses directly from waiting room demand. Flow validates room ownership, pre-populates course metadata from the waiting room, collects course details and session times, creates the activity and sessions, then fulfills the waiting room to link course and demand together.Frontend – Course Detail
course.html): New page displaying course metadata, session list, and host-specific delete controls for individual sessions or entire courses. Enrolled users see participation status; non-enrolled users see join CTAs; hosts see management actions.Backend – Durable Object & Deletion Endpoints
src/worker.py: New Durable Object class implementing/api/waitingroom/*routes for CRUD operations (get/list/create/join/leave/delete) and fulfillment (/api/waitingroom/fulfill), storing room state with participant tracking and fulfillment metadata.src/worker.py: NewPOST /api/activities/deleteandPOST /api/sessions/deleteendpoints enforcing host ownership, validating required IDs, and cascading deletes of dependent data with error handling.Impact
User Experience: Learners can now create learning requests (waiting rooms), see other open requests, join them to express interest, and view when instructors fulfill demand by creating courses. Instructors can browse waiting room demand, create courses directly from demand signals with pre-filled metadata, and manage course/session lifecycle with delete controls.
Functionality: Introduces a complete demand-to-supply pipeline where waiting room fulfillment triggers course creation and links the two entities, with polling-based real-time synchronization across list and detail views. Host-side deletion includes proper authorization checks and cascading cleanup of related sessions/enrollments.
Authorization: Creator-only actions (teach, delete rooms) and host-only actions (manage/delete courses and sessions) are validated server-side via token verification and ownership checks.