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
|
Warning Rate limit exceeded
⌛ 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 (3)
WalkthroughAdds a discussion/comments feature: DB schema for threaded comments, three new backend comment handlers (get/post/delete) implemented but not yet routed, and client-side UI + logic on the activity page to fetch, render, reply, and post comments. 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 comments + author data
Server->>Server: Decrypt comment bodies
Server-->>Client: Return comment thread (parent/replies)
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<br/>{body, parent_id?, Authorization: Bearer token}
Server->>Server: Validate auth, activity exists, 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: Re-fetch and re-render thread
end
rect rgba(200, 100, 0, 0.5)
Note over Client,DB: Delete Comment
Client->>Server: DELETE /api/comments/{commentId}<br/>{Authorization: Bearer token}
Server->>DB: Query comment + activity + user verification
Server->>Server: Check: comment owner OR activity host
alt Permission granted
Server->>DB: Delete comment
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
from
661e531 to
1e53385
Compare
ayesha1145
force-pushed
the
feat/activity-comments
branch
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 ✅ |
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:
Pull Request Summary: Add Activity Discussion/Comments System with Threaded Replies
Purpose
Adds a threaded discussion/comments system to activity pages, enabling enrolled users and hosts to discuss activities in real time with hierarchical parent-child comment relationships.
Key Changes
Database (schema.sql)
commentstable with support for threaded replies via optionalparent_idself-reference andON DELETE CASCADEbodystorage (at-rest encryption matching platform security model)activity_id,parent_id, anduser_idfor performancecreated_atandupdated_attimestampsBackend API (src/worker.py)
api_get_comments: Fetches and decrypts all comments for an activityapi_post_comment: Creates new comments with 2000-character limit and optionalparent_idfor replies; requires authenticationapi_delete_comment: Allows deletion by comment author, activity host, or admin_DDLschema to include comments table and indexesFrontend (public/course.html)
GET /api/activities/{actId}/commentsand posts viaPOSTwith bearer token authenticationparent_idescHtml()and conditional form visibility based on authenticationInput Validation
The API routes are not wired into the dispatcher. While all three handler functions are fully implemented and the database schema is complete, no route matchers have been added to the
_dispatchfunction. The_dispatchfunction will return404 "API endpoint not found"for all comment requests because the routes checking for/api/activities/:id/commentsand/api/comments/:idare missing. The feature is currently non-functional and requires routing to be added to_dispatchbefore the API endpoints can be accessed.