feat: add activity discussion/comments system with threaded replies#20
Open
ayesha1145 wants to merge 1 commit intoalphaonelabs:mainfrom
Open
feat: add activity discussion/comments system with threaded replies#20ayesha1145 wants to merge 1 commit intoalphaonelabs:mainfrom
ayesha1145 wants to merge 1 commit intoalphaonelabs:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds threaded comments: new Changes
Sequence DiagramsequenceDiagram
participant Client as Browser Client
participant Server as Worker (API)
participant DB as Database
rect rgba(0, 100, 200, 0.5)
Note over Client,DB: Load Comments Thread
Client->>Server: GET /api/activities/{actId}/comments
Server->>DB: Query comments by activity_id (+ users join)
DB-->>Server: Return encrypted comment rows + author refs
Server->>Server: Decrypt comment bodies and author names
Server-->>Client: Return comment thread (ordered by created_at)
Client->>Client: Render nested comments list
end
rect rgba(100, 150, 50, 0.5)
Note over Client,DB: Post New Comment
Client->>Server: POST /api/activities/{actId}/comments {body, parent_id?} + Authorization
Server->>Server: Validate auth, activity/enrollment, body length
alt parent_id provided
Server->>DB: Verify parent_id belongs to same activity
end
Server->>Server: Encrypt body
Server->>DB: Insert comment row
DB-->>Server: Confirm insert
Server-->>Client: Return created comment metadata
Client->>Client: Clear input, cancel reply, re-fetch thread
end
rect rgba(200, 100, 0, 0.5)
Note over Client,DB: Delete Comment
Client->>Server: DELETE /api/comments/{commentId} + Authorization
Server->>DB: Query comment + activity + author/host info
Server->>Server: Check permission (comment owner OR activity host)
alt Permission granted
Server->>DB: Delete comment (cascade replies)
DB-->>Server: Confirm deletion
Server-->>Client: Success response
else Permission denied
Server-->>Client: 403 Forbidden
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 6
🤖 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`:
- Around line 308-313: The inline script references DOM elements ('comment-form'
and 'comment-login-cta') before the comments markup exists, causing
getElementById to return null and abort execution; wrap the block that checks
token/actId and calls loadComments() (and any initialization that depends on DOM
nodes, e.g., loadActivity()) in a DOMContentLoaded listener or defer execution
until the DOM is ready, so element accesses
(document.getElementById('comment-form') and
document.getElementById('comment-login-cta')) are safe; alternatively move the
comments section markup above the script so the elements exist before the script
runs.
- Around line 326-333: The composer lacks an accessible label and the buttons
miss explicit types: add a programmatic label for the textarea (e.g., create a
<label> or use aria-label/aria-labelledby referencing the element with
id="comment-input" or the reply indicator id="reply-indicator") so the textarea
with id="comment-input" is properly announced, and set type="button" on both the
cancel button (which calls cancelReply()) and the Post Comment button (which
calls postComment()) to prevent accidental form submission if wrapped in a form.
- Around line 255-257: Replace the inline onclick reply construction (replyBtn)
with a safe data-attribute approach and a delegated click handler: stop emitting
"onclick" that calls startReply(...) and instead render the button with data
attributes like data-comment-id and data-comment-author (use escHtml for
HTML-escaping only), then add a single delegated click listener that reads
event.target.dataset and calls startReply(id, author). Also fix the DOM timing
errors by deferring code that uses document.getElementById('#comment-form') and
'#comment-login-cta' — either move the script to just before </body> or wrap
those calls in DOMContentLoaded (or a similar defer) so they never run on null.
Finally, add a proper <label> for the comment textarea and ensure all
form-action buttons explicitly include type="button" (or type="submit" where
appropriate) to prevent unintended submissions.
In `@schema.sql`:
- Around line 103-108: The comments.parent_id FK currently self-references
comments(id) without delete semantics, causing either FK errors or orphaned
replies when the delete API removes only one row; pick and apply a consistent
policy: either add "ON DELETE CASCADE" to the FOREIGN KEY (parent_id) REFERENCES
comments(id) in the schema and ensure the comment-deletion code (the delete
comment handler that currently deletes a single row) performs one-row deletes
(the DB will cascade the subtree), or add "ON DELETE SET NULL" in the schema and
update the delete handler to set child comments' parent_id to NULL (or run an
update query to nullify children) before deleting the parent; also update any
migration SQL and the corresponding logic in the comment-deletion function so
the DB schema and worker deletion semantics match.
In `@src/worker.py`:
- Around line 1318-1323: The current authorization grants any user with
user.get("role") == "host" global delete rights (is_admin), which duplicates the
activity host check and allows unrelated hosts to delete comments; remove the
is_admin branch or change it to check for an explicit admin role (e.g.,
user.get("role") == "admin") and ensure tokens actually mint that admin role;
update the authorization logic around is_owner, is_host, is_admin (and the
err("Permission denied", 403) return) so only the comment owner, the activity
host, or a true admin can delete comments.
- Around line 1233-1326: Dispatch currently doesn't route the new comment
endpoints, so add matches in _dispatch() to call api_get_comments,
api_post_comment, and api_delete_comment. Specifically: when the path matches
GET /api/activities/:id/comments call api_get_comments(req, env, activity_id,
enc_key); when it matches POST /api/activities/:id/comments call
api_post_comment(req, env, activity_id, user, enc_key); and when it matches
DELETE /api/comments/:id call api_delete_comment(req, env, comment_id, user).
Ensure you extract activity_id/comment_id from the route, and pass the same env,
req, enc_key and user objects used elsewhere in _dispatch() so the functions
api_get_comments, api_post_comment and api_delete_comment are reachable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ab0765c9-c3e2-4d12-b671-ed470c6367ed
📒 Files selected for processing (3)
public/course.htmlschema.sqlsrc/worker.py
ayesha1145
force-pushed
the
feat/activity-comments
branch
from
bd5e152 to
661e531
Compare
Contributor
Author
|
Addressed CodeRabbit feedback:
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
public/course.html (2)
255-257:⚠️ Potential issue | 🔴 CriticalThe XSS vulnerability in the reply button remains unfixed.
The
escHtml()function escapes HTML entities (<,>,&,") but does not escape single quotes. When an author's name contains a single quote (e.g., "O'Brien"), the inlineonclickbreaks:<button onclick="startReply('123', 'O'Brien')">This can be exploited for script injection. The delegated click handler at lines 308-312 was added but never used—the buttons don't have the
reply-btnclass.🔧 Suggested fix: Use data attributes with delegated handler
function renderComment(c, byParent) { const replies = (byParent[c.id] || []).map(r => renderComment(r, byParent)).join(''); const initial = (c.author || '?')[0].toUpperCase(); const date = new Date(c.created_at).toLocaleDateString(); const replyBtn = token - ? '<button onclick="startReply(\'' + c.id + '\', \'' + escHtml(c.author) + '\')" class="text-xs text-indigo-500 hover:underline">Reply</button>' + ? '<button type="button" class="reply-btn text-xs text-indigo-500 hover:underline" data-id="' + escHtml(c.id) + '" data-author="' + escHtml(c.author) + '">Reply</button>' : '';The existing delegated handler at lines 308-312 will then work correctly.
As per coding guidelines,
**/*.html: 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` around lines 255 - 257, The reply button uses inline onclick with escHtml for the author which doesn't escape single quotes, causing an XSS vector; instead render the button without inline JS and put the author/id into data attributes (e.g., data-comment-id and data-author) and add the "reply-btn" class so the existing delegated handler (the click listener that expects ".reply-btn") can read dataset.commentId/dataset.author and call startReply safely; update where the button is created (the expression that currently builds the '<button onclick="startReply(...)"...'> string) to emit data- attributes and class="reply-btn" and remove the inline onclick, keeping escHtml for HTML content where needed.
339-341: 🧹 Nitpick | 🔵 TrivialPlace the label before the textarea for conventional accessibility.
While placing a
<label>after its associated input is valid HTML, conventional practice (and many screen readers' expectations) is to place labels before inputs. Consider moving line 341 above line 339.📝 Suggested reordering
<div id="reply-indicator" class="hidden text-xs text-indigo-600 mb-2"> Replying to a comment — <button type="button" onclick="cancelReply()" class="underline">cancel</button> </div> + <label for="comment-input" class="sr-only">Write a comment</label> <textarea id="comment-input" rows="3" placeholder="Write a comment..." class="w-full border border-slate-200 rounded-xl px-4 py-2 text-sm text-slate-700 focus:outline-none focus:ring-2 focus:ring-indigo-300 resize-none"></textarea> - <label for="comment-input" class="sr-only">Write a comment</label>As per coding guidelines,
**/*.html: Review HTML templates for accessibility (ARIA attributes, semantic elements).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/course.html` around lines 339 - 341, Move the visually hidden label for the comment input so it precedes the textarea element to follow conventional accessibility order: place the <label for="comment-input" class="sr-only">Write a comment</label> before the <textarea id="comment-input" ...> element (referencing the id "comment-input" and the label's for attribute) and ensure the for/id pairing remains unchanged.src/worker.py (1)
1225-1322:⚠️ Potential issue | 🔴 CriticalThese handlers are unreachable—routing is missing from
_dispatch().The three comment handlers (
api_get_comments,api_post_comment,api_delete_comment) are defined but never called. The_dispatch()function (lines 1155-1222) doesn't include routes for:
GET /api/activities/:id/commentsPOST /api/activities/:id/commentsDELETE /api/comments/:idFrontend calls to these endpoints will receive
{"error": "API endpoint not found"}(404).🔧 Add routing in `_dispatch()` before line 1220
# Comments m = re.fullmatch(r"/api/activities/([A-Za-z0-9_-]+)/comments", path) if m and method == "GET": return await api_get_comments(request, env, m.group(1), env.ENCRYPTION_KEY) if m and method == "POST": user = verify_token(request.headers.get("Authorization"), env.JWT_SECRET) return await api_post_comment(request, env, m.group(1), user, env.ENCRYPTION_KEY) m = re.fullmatch(r"/api/comments/([A-Za-z0-9_-]+)", path) if m and method == "DELETE": user = verify_token(request.headers.get("Authorization"), env.JWT_SECRET) return await api_delete_comment(request, env, m.group(1), user)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker.py` around lines 1225 - 1322, The comment handlers api_get_comments, api_post_comment, and api_delete_comment are never routed because _dispatch() lacks matching routes; add route checks in _dispatch() to match GET /api/activities/:id/comments -> call api_get_comments(request, env, activity_id, env.ENCRYPTION_KEY), POST /api/activities/:id/comments -> verify_token(...) then call api_post_comment(request, env, activity_id, user, env.ENCRYPTION_KEY), and DELETE /api/comments/:id -> verify_token(...) then call api_delete_comment(request, env, comment_id, user); place these checks alongside the other API route matches in _dispatch() (use the same regex style used elsewhere to extract the id).
🤖 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`:
- Around line 308-312: The delegated click listener on document checks for
elements with class 'reply-btn' and calls startReply(id, author), but
renderComment() currently renders reply buttons with inline onclicks and no
'reply-btn' class or dataset attributes; update renderComment() to remove the
inline onclick, add class="reply-btn", and include data-id and data-author
attributes on the reply button so the existing
document.addEventListener('click', ...) can find
e.target.classList.contains('reply-btn') and call startReply using
e.target.dataset.id and e.target.dataset.author.
In `@src/worker.py`:
- Around line 367-382: The _DDL definition for the comments table is missing the
ON DELETE CASCADE on the parent_id foreign key, causing schema drift versus
schema.sql; update the comments table DDL in the _DDL variable so the FOREIGN
KEY (parent_id) REFERENCES comments(id) clause includes ON DELETE CASCADE (i.e.,
change the parent_id FK in the comments CREATE TABLE statement inside _DDL to
"FOREIGN KEY (parent_id) REFERENCES comments(id) ON DELETE CASCADE") to ensure
child replies are removed when a parent comment is deleted.
- Line 1230: The req parameter is unused in api_get_comments and
api_delete_comment; to silence linters and clarify intent, rename the parameter
to _req in both function signatures (api_get_comments and api_delete_comment) or
remove it if the handler interface permits—ensure you only change the parameter
name in the function definitions and not any internal references (there are
none), and run tests/lint to confirm no callers rely on the original name.
- Around line 1293-1296: Wrap the database insert (the await
env.DB.prepare(...).bind(cid, activity_id, user["id"], encrypt(text, enc_key),
parent_id).run() call) in a try/except block, call capture_exception(e) in the
except to log the error, and return or raise the same kind of graceful error
response your other handlers use (e.g., the pattern used in api_register /
api_create_activity) so callers get a consistent error path; ensure
capture_exception is imported where needed and preserve the existing variables
cid, activity_id, user["id"], encrypt(text, enc_key), and parent_id when moving
the call into the try block.
- Around line 1248-1258: Replace the imperative loop that builds comments with a
list comprehension for clarity and concision: construct comments = [ { "id":
r["id"], "body": decrypt(r["body"], enc_key), "parent_id": r["parent_id"],
"created_at": r["created_at"], "updated_at": r["updated_at"], "user_id":
r["user_id"], "author": decrypt(r["author_name"], enc_key) } for r in
(rows.results or []) ], preserving use of decrypt and enc_key and the same keys
from each r.
---
Duplicate comments:
In `@public/course.html`:
- Around line 255-257: The reply button uses inline onclick with escHtml for the
author which doesn't escape single quotes, causing an XSS vector; instead render
the button without inline JS and put the author/id into data attributes (e.g.,
data-comment-id and data-author) and add the "reply-btn" class so the existing
delegated handler (the click listener that expects ".reply-btn") can read
dataset.commentId/dataset.author and call startReply safely; update where the
button is created (the expression that currently builds the '<button
onclick="startReply(...)"...'> string) to emit data- attributes and
class="reply-btn" and remove the inline onclick, keeping escHtml for HTML
content where needed.
- Around line 339-341: Move the visually hidden label for the comment input so
it precedes the textarea element to follow conventional accessibility order:
place the <label for="comment-input" class="sr-only">Write a comment</label>
before the <textarea id="comment-input" ...> element (referencing the id
"comment-input" and the label's for attribute) and ensure the for/id pairing
remains unchanged.
In `@src/worker.py`:
- Around line 1225-1322: The comment handlers api_get_comments,
api_post_comment, and api_delete_comment are never routed because _dispatch()
lacks matching routes; add route checks in _dispatch() to match GET
/api/activities/:id/comments -> call api_get_comments(request, env, activity_id,
env.ENCRYPTION_KEY), POST /api/activities/:id/comments -> verify_token(...) then
call api_post_comment(request, env, activity_id, user, env.ENCRYPTION_KEY), and
DELETE /api/comments/:id -> verify_token(...) then call
api_delete_comment(request, env, comment_id, user); place these checks alongside
the other API route matches in _dispatch() (use the same regex style used
elsewhere to extract the id).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f69f08e-5771-4740-a019-6f98e9380b8e
📒 Files selected for processing (3)
public/course.htmlschema.sqlsrc/worker.py
ayesha1145
force-pushed
the
feat/activity-comments
branch
2 times, most recently
from
1e53385 to
f456156
Compare
Contributor
Author
|
Addressed latest CodeRabbit feedback:
|
Contributor
Author
|
@A1L13N This PR is ready for review — all feedback has been addressed ✅ |
ayesha1145
force-pushed
the
feat/activity-comments
branch
from
f456156 to
7d14249
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/worker.py (1)
1310-1313:⚠️ Potential issue | 🔴 CriticalCritical: Comment API routes are not wired into
_dispatch().The three comment handlers (
api_get_comments,api_post_comment,api_delete_comment) exist but are never called from_dispatch(). All requests to/api/activities/:id/commentsand/api/comments/:idwill return"API endpoint not found"(404).🔧 Suggested routing additions (insert before line 1313)
if path == "/api/admin/table-counts" and method == "GET": return await api_admin_table_counts(request, env) + # Comments routes + m = re.fullmatch(r"/api/activities/([A-Za-z0-9_-]+)/comments", path) + if m and method == "GET": + return await api_get_comments(request, env, m.group(1), env.ENCRYPTION_KEY) + if m and method == "POST": + user = verify_token(request.headers.get("Authorization"), env.JWT_SECRET) + return await api_post_comment(request, env, m.group(1), user, env.ENCRYPTION_KEY) + + m = re.fullmatch(r"/api/comments/([A-Za-z0-9_-]+)", path) + if m and method == "DELETE": + user = verify_token(request.headers.get("Authorization"), env.JWT_SECRET) + return await api_delete_comment(request, env, m.group(1), user) + return err("API endpoint not found", 404)Run the following script to confirm the handlers are defined but not called:
#!/bin/bash # Check that handlers exist echo "=== Handlers defined ===" rg -n "^async def api_(get|post|delete)_comment" src/worker.py echo "" echo "=== References in _dispatch ===" rg -n "api_get_comments|api_post_comment|api_delete_comment" src/worker.py | grep -v "^.*:async def"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker.py` around lines 1310 - 1313, The three comment handlers are never routed in _dispatch; add route checks before the final "return err(...)" to call api_get_comments, api_post_comment, and api_delete_comment. Specifically, in _dispatch() detect GET requests matching "/api/activities/<id>/comments" and call await api_get_comments(request, env, activity_id), detect POST to the same path and call await api_post_comment(request, env, activity_id), and detect DELETE requests matching "/api/comments/<id>" and call await api_delete_comment(request, env, comment_id); extract the id from the path segments (or via simple split/regex) and ensure you await the handler results and return them. Ensure you reference the existing function names api_get_comments, api_post_comment, api_delete_comment and place these checks immediately before the current fallback return err("API endpoint not found", 404).
🤖 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`:
- Around line 331-350: The comments section (element with id="comments-section")
is currently outside the <main> container so it doesn't inherit the page's
centered max-width styling; move the entire <div id="comments-section"> block
into the <main> element (place it before <main>'s closing tag, alongside the
other content cards) so it benefits from the same max-w-7xl mx-auto layout and
visual consistency.
In `@src/worker.py`:
- Around line 1341-1352: The current list comprehension uses the deprecated sync
decrypt() (which raises RuntimeError); replace those calls with the async
decrypt_aes() and refactor the comprehension into an explicit loop because you
cannot await inside a list comprehension: iterate over (rows.results or []),
build each comment dict setting "body" to await decrypt_aes(r["body"], enc_key)
and "author" to await decrypt_aes(r["author_name"], enc_key), and append to the
comments list so comments is populated with the awaited decrypted values.
- Around line 1386-1394: The encrypt() call used in the api_post_comment flow is
a deprecated synchronous shim that raises RuntimeError; replace the sync
encrypt() with the async encrypt_aes() and await its result before binding to
the DB insert. Specifically, in the block that calls
env.DB.prepare(...).bind(... encrypt(text, enc_key) ...).run(), call encrypted =
await encrypt_aes(text, enc_key) (or the project’s async encrypt helper), then
bind encrypted in place of encrypt(...); ensure api_post_comment imports/uses
encrypt_aes and that the await is inside the try/except so exceptions are
captured by capture_exception.
---
Duplicate comments:
In `@src/worker.py`:
- Around line 1310-1313: The three comment handlers are never routed in
_dispatch; add route checks before the final "return err(...)" to call
api_get_comments, api_post_comment, and api_delete_comment. Specifically, in
_dispatch() detect GET requests matching "/api/activities/<id>/comments" and
call await api_get_comments(request, env, activity_id), detect POST to the same
path and call await api_post_comment(request, env, activity_id), and detect
DELETE requests matching "/api/comments/<id>" and call await
api_delete_comment(request, env, comment_id); extract the id from the path
segments (or via simple split/regex) and ensure you await the handler results
and return them. Ensure you reference the existing function names
api_get_comments, api_post_comment, api_delete_comment and place these checks
immediately before the current fallback return err("API endpoint not found",
404).
🪄 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: 766cb005-b8a4-4ae2-a6da-8ce3bd10f095
📒 Files selected for processing (3)
public/course.htmlschema.sqlsrc/worker.py
ayesha1145
force-pushed
the
feat/activity-comments
branch
from
7d14249 to
44a6430
Compare
Contributor
Author
|
Fixed critical: replaced sync encrypt()/decrypt() shims with await encrypt_aes()/await decrypt_aes() in both api_get_comments and api_post_comment — prevents RuntimeError 500 on every comment request. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
public/course.html (1)
229-257:⚠️ Potential issue | 🔴 CriticalEscape
data-authoras an attribute, not as text.
escHtml()does not encode double quotes, so a display name likeBob" autofocus onfocus="...can break out ofdata-authorwhen this string is injected withinnerHTML.As per coding guidelines, `**/*.html`: Review HTML templates for accessibility (ARIA attributes, semantic elements), XSS risks from unescaped user content, and proper use of template inheritance.Minimal safe fix
let replyToId = null; function escHtml(s) { const d = document.createElement('div'); d.textContent = s||''; return d.innerHTML; } + function escAttr(s) { + return String(s ?? '') + .replace(/&/g, '&') + .replace(/"/g, '"') + .replace(/'/g, ''') + .replace(/</g, '<') + .replace(/>/g, '>'); + } async function loadComments() { try { const res = await fetch('/api/activities/' + actId + '/comments'); @@ const date = new Date(c.created_at).toLocaleDateString(); const replyBtn = token - ? '<button type="button" class="reply-btn text-xs text-indigo-500 hover:underline" data-id="' + c.id + '" data-author="' + escHtml(c.author) + '">Reply</button>' + ? '<button type="button" class="reply-btn text-xs text-indigo-500 hover:underline" data-id="' + c.id + '" data-author="' + escAttr(c.author) + '">Reply</button>' : '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/course.html` around lines 229 - 257, The reply button HTML in renderComment builds a data-author attribute using escHtml which only escapes text nodes and can be broken by quotes; update renderComment (the replyBtn construction) to safely escape attribute values (or avoid string-built attributes entirely) by either: 1) using a proper attribute-escaping function for data-author that encodes quotes and other attribute-sensitive characters, or 2) creating the button via DOM APIs (document.createElement('button'), setAttribute('data-id', c.id), setAttribute('data-author', c.author) or use element.dataset.author = c.author) and appending it, ensuring escHtml remains for text content only. Ensure changes reference escHtml and renderComment/replyBtn so the data-author value cannot break out of the attribute.src/worker.py (1)
1261-1313:⚠️ Potential issue | 🔴 CriticalRoute the new comments endpoints through
_dispatch().The three handlers below are still unreachable because
/api/activities/:id/commentsand/api/comments/:idnever match here, so the frontend will only get 404s.Suggested dispatch wiring
if path == "/api/activities" and method == "POST": return await api_create_activity(request, env) m = re.fullmatch(r"/api/activities/([A-Za-z0-9_-]+)", path) if m and method == "GET": return await api_get_activity(m.group(1), request, env) + + m = re.fullmatch(r"/api/activities/([A-Za-z0-9_-]+)/comments", path) + if m and method == "GET": + return await api_get_comments(request, env, m.group(1), env.ENCRYPTION_KEY) + if m and method == "POST": + user = verify_token(request.headers.get("Authorization"), env.JWT_SECRET) + return await api_post_comment(request, env, m.group(1), user, env.ENCRYPTION_KEY) + + m = re.fullmatch(r"/api/comments/([A-Za-z0-9_-]+)", path) + if m and method == "DELETE": + user = verify_token(request.headers.get("Authorization"), env.JWT_SECRET) + return await api_delete_comment(request, env, m.group(1), user) if path == "/api/join" and method == "POST": return await api_join(request, env)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker.py` around lines 1261 - 1313, The routing block never forwards requests for the new comment endpoints (e.g. "/api/activities/<id>/comments" and "/api/comments/<id>") to the comment handlers, so those routes return 404; update the "/api/" routing to dispatch comment routes via the existing _dispatch() mechanism (or explicitly match the patterns and call the comment handlers) so requests hit the comment logic—specifically, add a call to _dispatch(request, env) (or pattern-match and call api_list_comments/api_create_comment/api_delete_comment) for the "/api/activities/<id>/comments" and "/api/comments/<id>" patterns before returning the generic "API endpoint not found".
🤖 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`:
- Around line 244-307: The template is missing the DOM elements expected by the
comment script (IDs 'comments-list', 'comment-form', 'comment-login-cta',
'reply-indicator', 'comment-input'), causing loadComments, renderComment,
startReply and postComment to fail or never show a discussion UI; add a comments
block to the HTML that includes a container with id="comments-list", a form with
id="comment-form" and a textarea/input id="comment-input" (or a login CTA
element id="comment-login-cta" when unauthenticated), and an element
id="reply-indicator" for reply state, ensuring the form's submit/button wiring
matches postComment and reply buttons use the reply-btn class and
data-id/data-author attributes used by renderComment and startReply.
In `@src/worker.py`:
- Around line 1368-1385: The code currently assumes body["body"] and parent_id
are strings which causes a 500 on non-string inputs; update the handler after
parse_json_object to explicitly validate types: check that body.get("body") is a
str before calling .strip() and return err("Comment body must be a string", 400)
if not; similarly validate parent_id (e.g., ensure isinstance(parent_id, (str,
int)) or explicitly require str) before using it in the DB query and return
err("parent_id must be a string or integer", 400) if invalid; keep using
parse_json_object, err, activity_id and env.DB.prepare(...) as in the original
code paths, and convert/cast validated values as needed prior to .strip() and
.bind().
- Around line 1356-1366: The api_post_comment handler only checks auth and
activity existence but not that the authenticated user is a member of the
activity; add an authorization gate in api_post_comment that queries the
membership relationship (e.g., SELECT 1 FROM activity_memberships OR
participants WHERE activity_id = ? AND user_id = ?) using env.DB.prepare with
bound parameters to avoid SQL injection, return a 403 (or err("Not a member",
403)) if no row is found, and log the failed authorization; ensure this check
runs after the existing activity existence check and before any insert logic so
only members can post comments.
---
Duplicate comments:
In `@public/course.html`:
- Around line 229-257: The reply button HTML in renderComment builds a
data-author attribute using escHtml which only escapes text nodes and can be
broken by quotes; update renderComment (the replyBtn construction) to safely
escape attribute values (or avoid string-built attributes entirely) by either:
1) using a proper attribute-escaping function for data-author that encodes
quotes and other attribute-sensitive characters, or 2) creating the button via
DOM APIs (document.createElement('button'), setAttribute('data-id', c.id),
setAttribute('data-author', c.author) or use element.dataset.author = c.author)
and appending it, ensuring escHtml remains for text content only. Ensure changes
reference escHtml and renderComment/replyBtn so the data-author value cannot
break out of the attribute.
In `@src/worker.py`:
- Around line 1261-1313: The routing block never forwards requests for the new
comment endpoints (e.g. "/api/activities/<id>/comments" and
"/api/comments/<id>") to the comment handlers, so those routes return 404;
update the "/api/" routing to dispatch comment routes via the existing
_dispatch() mechanism (or explicitly match the patterns and call the comment
handlers) so requests hit the comment logic—specifically, add a call to
_dispatch(request, env) (or pattern-match and call
api_list_comments/api_create_comment/api_delete_comment) for the
"/api/activities/<id>/comments" and "/api/comments/<id>" patterns before
returning the generic "API endpoint not found".
🪄 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: 8c4f892e-4f1f-4d2b-91a6-faf3409278ed
📒 Files selected for processing (3)
public/course.htmlschema.sqlsrc/worker.py
ayesha1145
force-pushed
the
feat/activity-comments
branch
from
44a6430 to
e15bff5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/worker.py (1)
1291-1313:⚠️ Potential issue | 🔴 CriticalCritical: Comment endpoints are not wired into
_dispatch()— all requests will 404.The three comment handlers (
api_get_comments,api_post_comment,api_delete_comment) are implemented below but never routed here. Any frontend call to/api/activities/:id/commentsor/api/comments/:idfalls through to line 1313 and returns"API endpoint not found".🐛 Proposed routing fix
Add these routes before the final
return err(...)on line 1313:if path == "/api/admin/table-counts" and method == "GET": return await api_admin_table_counts(request, env) + # Comment endpoints + m = re.fullmatch(r"/api/activities/([A-Za-z0-9_-]+)/comments", path) + if m and method == "GET": + return await api_get_comments(request, env, m.group(1), env.ENCRYPTION_KEY) + if m and method == "POST": + user = verify_token(request.headers.get("Authorization"), env.JWT_SECRET) + return await api_post_comment(request, env, m.group(1), user, env.ENCRYPTION_KEY) + + m = re.fullmatch(r"/api/comments/([A-Za-z0-9_-]+)", path) + if m and method == "DELETE": + user = verify_token(request.headers.get("Authorization"), env.JWT_SECRET) + return await api_delete_comment(request, env, m.group(1), user) + return err("API endpoint not found", 404)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker.py` around lines 1291 - 1313, The _dispatch() routing misses the comment endpoints so requests to /api/activities/:id/comments and /api/comments/:id always 404; update the routing block in _dispatch() to invoke api_get_comments for GET on /api/activities/{id}/comments, api_post_comment for POST on /api/activities/{id}/comments, and api_delete_comment for DELETE on /api/comments/{id} (use the same URL parsing style as the existing activity route, e.g. re.fullmatch for the activity ID) and insert these checks just before the final return err("API endpoint not found", 404).
🤖 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`:
- Around line 226-329: The template is missing the Comments HTML that the JS
expects; add a comments block inside the main content (before the closing
</main>) containing elements with the exact IDs used by the script:
comments-list, comment-form, comment-login-cta, reply-indicator, and
comment-input; ensure comment-form is initially hidden (so token logic shows
it), include a labelled textarea (use an sr-only label for accessibility), a
Post Comment button that calls postComment(), and a login CTA shown when not
logged in—this will allow renderComments(), startReply()/cancelReply(),
postComment() and the DOMContentLoaded token logic to function.
In `@src/worker.py`:
- Around line 1323-1360: The handler api_get_comments currently references
undefined user and uses await inside a list comprehension; fix by changing the
signature to accept user (e.g., async def api_get_comments(_req, env,
activity_id: str, user, enc_key: str)), update the caller/dispatch to pass the
verified user (use verify_token(...) and pass that user into api_get_comments),
and replace the list comprehension with an explicit async-safe loop that awaits
decrypt_aes for body and author_name (e.g., build comments = [] then for r in
(rows.results or []): decrypted_body = await decrypt_aes(...); decrypted_author
= await decrypt_aes(...); comments.append({...})), keeping existing checks that
use user["id"] and returning ok(comments).
---
Outside diff comments:
In `@src/worker.py`:
- Around line 1291-1313: The _dispatch() routing misses the comment endpoints so
requests to /api/activities/:id/comments and /api/comments/:id always 404;
update the routing block in _dispatch() to invoke api_get_comments for GET on
/api/activities/{id}/comments, api_post_comment for POST on
/api/activities/{id}/comments, and api_delete_comment for DELETE on
/api/comments/{id} (use the same URL parsing style as the existing activity
route, e.g. re.fullmatch for the activity ID) and insert these checks just
before the final return err("API endpoint not found", 404).
🪄 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: 7f2ffd3b-cd6d-4ffb-be58-0250643b628a
📒 Files selected for processing (3)
public/course.htmlschema.sqlsrc/worker.py
Contributor
Author
|
Addressed latest CodeRabbit feedback:
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/worker.py (2)
17-19:⚠️ Potential issue | 🔴 CriticalWire these routes into
_dispatch().These endpoints are documented here and their handlers exist below, but
_dispatch()never matches them. Right now every comment GET/POST, plus everyDELETE /api/comments/:id, falls through to the generic 404 path.Suggested `_dispatch()` wiring
m = re.fullmatch(r"/api/activities/([A-Za-z0-9_-]+)", path) if m and method == "GET": return await api_get_activity(m.group(1), request, env) + + m = re.fullmatch(r"/api/activities/([A-Za-z0-9_-]+)/comments", path) + if m and method == "GET": + return await api_get_comments(request, env, m.group(1), env.ENCRYPTION_KEY) + if m and method == "POST": + user = verify_token(request.headers.get("Authorization"), env.JWT_SECRET) + return await api_post_comment(request, env, m.group(1), user, env.ENCRYPTION_KEY) + + m = re.fullmatch(r"/api/comments/([A-Za-z0-9_-]+)", path) + if m and method == "DELETE": + user = verify_token(request.headers.get("Authorization"), env.JWT_SECRET) + return await api_delete_comment(request, env, m.group(1), user)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker.py` around lines 17 - 19, The _dispatch() function currently never matches the activity comment and comment-delete endpoints; update _dispatch() to recognize and route the following patterns to the existing handlers defined below: GET /api/activities/:id/comments -> call the handler for listing activity comments (e.g. list_comments_for_activity), POST /api/activities/:id/comments -> call the handler that creates posts for an activity (e.g. post_comment_for_activity) and enforce authentication as the handler expects, and DELETE /api/comments/:id -> call the handler that deletes a comment by id (e.g. delete_comment_by_id) and enforce owner|host authorization; ensure the route pattern matching and parameter extraction for :id are consistent with how the handlers expect to receive the activity/comment id.
1323-1337:⚠️ Potential issue | 🔴 Critical
api_get_comments()will 500 before it returns anything.Line 1332 and Line 1335 read
user["id"], butuseris not defined in this function or passed by_dispatch(). The browser also calls this endpoint without an auth header, so even after fixing theNameError, the current membership gate would still reject anonymous reads.Suggested fix if reads are meant to stay public
async def api_get_comments(_req, env, activity_id: str, enc_key: str): """GET /api/activities/:id/comments — list comments for an activity.""" # Check activity exists act = await env.DB.prepare( "SELECT id, host_id FROM activities WHERE id = ?" ).bind(activity_id).first() if not act: return err("Activity not found", 404) - # Check user is enrolled or is host - if act["host_id"] != user["id"]: - enr = await env.DB.prepare( - "SELECT id FROM enrollments WHERE activity_id=? AND user_id=? AND status='active'" - ).bind(activity_id, user["id"]).first() - if not enr: - return err("You must be enrolled to comment", 403) rows = await env.DB.prepare( "SELECT c.id, c.body, c.parent_id, c.created_at, c.updated_at, "🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker.py` around lines 1323 - 1337, api_get_comments currently references user["id"] but no user is defined, causing a NameError and blocking anonymous reads; fix by first obtaining the authenticated user (if any) and then enforcing the enrollment/host check only when a user is present. Concretely, in api_get_comments retrieve the authenticated user id from the request/context (e.g., a decoded auth token or request ctx) into a local user_id variable (or None if unauthenticated), then change the host/enrollment gate to: if act["host_id"] != user_id: if user_id is None return/public allow (if reads are intended public) else query enrollments with .bind(activity_id, user_id) and return 403 when no active enrollment. Update references to user["id"] in this function accordingly (api_get_comments, activity/enrollments checks).public/course.html (1)
230-323:⚠️ Potential issue | 🔴 CriticalRender the discussion markup before this script tries to use it.
This script assumes
#comments-list,#comment-form,#comment-login-cta,#reply-indicator, and#comment-inputalready exist, but the template never renders them. That makes the feature invisible, and onceloadComments()succeeds, Line 244 will try to writeinnerHTMLonnull.Suggested markup to insert inside ``
<section id="comments-section" class="mt-8 bg-white rounded-2xl shadow-sm border border-slate-100 p-6"> <h2 class="font-bold text-slate-800 text-lg mb-4">Discussion</h2> <div id="comments-list" class="space-y-4 mb-6"> <p class="text-slate-400 text-sm">Loading comments...</p> </div> <div id="comment-form" class="hidden"> <div id="reply-indicator" class="hidden text-xs text-indigo-600 mb-2"></div> <label for="comment-input" class="sr-only">Write a comment</label> <textarea id="comment-input" rows="3" placeholder="Write a comment..." class="w-full border border-slate-200 rounded-xl px-4 py-2 text-sm text-slate-700 focus:outline-none focus:ring-2 focus:ring-indigo-300 resize-none"></textarea> <button type="button" onclick="postComment()" class="mt-2 bg-indigo-600 hover:bg-indigo-700 text-white font-semibold px-5 py-2 rounded-xl text-sm transition"> Post Comment </button> </div> <div id="comment-login-cta" class="hidden text-center py-4"> <p class="text-slate-500 text-sm mb-2">Want to join the discussion?</p> <a href="/login.html" class="text-indigo-600 font-semibold hover:underline">Login to comment</a> </div> </section>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/course.html` around lines 230 - 323, The page is missing the discussion HTML that the script expects, causing document.getElementById calls in loadComments, renderComments, postComment and startReply to return null; add the suggested discussion markup into the template (inside <main>) so elements with IDs comments-list, comment-form, comment-login-cta, reply-indicator and comment-input exist (with the comment-form and comment-login-cta initially hidden and comments-list showing a "Loading comments..." placeholder), and ensure the markup is placed before this script executes (or keep the DOMContentLoaded guard) so loadComments() and the click handlers can safely query and update those elements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@public/course.html`:
- Around line 230-323: The page is missing the discussion HTML that the script
expects, causing document.getElementById calls in loadComments, renderComments,
postComment and startReply to return null; add the suggested discussion markup
into the template (inside <main>) so elements with IDs comments-list,
comment-form, comment-login-cta, reply-indicator and comment-input exist (with
the comment-form and comment-login-cta initially hidden and comments-list
showing a "Loading comments..." placeholder), and ensure the markup is placed
before this script executes (or keep the DOMContentLoaded guard) so
loadComments() and the click handlers can safely query and update those
elements.
In `@src/worker.py`:
- Around line 17-19: The _dispatch() function currently never matches the
activity comment and comment-delete endpoints; update _dispatch() to recognize
and route the following patterns to the existing handlers defined below: GET
/api/activities/:id/comments -> call the handler for listing activity comments
(e.g. list_comments_for_activity), POST /api/activities/:id/comments -> call the
handler that creates posts for an activity (e.g. post_comment_for_activity) and
enforce authentication as the handler expects, and DELETE /api/comments/:id ->
call the handler that deletes a comment by id (e.g. delete_comment_by_id) and
enforce owner|host authorization; ensure the route pattern matching and
parameter extraction for :id are consistent with how the handlers expect to
receive the activity/comment id.
- Around line 1323-1337: api_get_comments currently references user["id"] but no
user is defined, causing a NameError and blocking anonymous reads; fix by first
obtaining the authenticated user (if any) and then enforcing the enrollment/host
check only when a user is present. Concretely, in api_get_comments retrieve the
authenticated user id from the request/context (e.g., a decoded auth token or
request ctx) into a local user_id variable (or None if unauthenticated), then
change the host/enrollment gate to: if act["host_id"] != user_id: if user_id is
None return/public allow (if reads are intended public) else query enrollments
with .bind(activity_id, user_id) and return 403 when no active enrollment.
Update references to user["id"] in this function accordingly (api_get_comments,
activity/enrollments checks).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c7cab737-7773-4c80-9e61-10b221988bac
📒 Files selected for processing (3)
public/course.htmlschema.sqlsrc/worker.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a threaded discussion/comments system to activity pages, allowing enrolled users and hosts to discuss activities in real time.
Changes:
Features:
Purpose
Key changes
Database
Backend (src/worker.py)
Frontend (public/course.html)
Behavior & permissions
Code quality & fixes
Potential impact