Study Groups#75
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:
WalkthroughThis PR adds study-group support across schema, backend API handling, worker routing, and the activity creation UI. It introduces membership and invitation tables, implements study-group lifecycle and access flows, wires request dispatch to a dedicated module, and adds a ChangesStudy Groups Feature
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/invitations.html`:
- Around line 87-104: The fade-out in respond() is ineffective because render()
in invitations.html rebuilds the list before the animation runs, detaching the
passed article node. Update respond() so the optimistic removal and render()
happen without trying to animate a stale DOM node, and remove the
article.classList.add('opacity-50')/article.remove() path or move the animation
to the live element before render() replaces it. Also drop the redundant
loadInvites() call on the success path since the local invites state is already
updated by the optimistic splice.
- Around line 55-73: The dynamic invite list in render() updates without
notifying assistive tech, so screen reader users miss accept/decline removals.
Add a polite live region to the `#invite-list` container in the invitations
template so updates are announced when render() replaces its contents, keeping
the existing list rendering logic unchanged.
In `@public/study-group-detail.html`:
- Around line 33-34: The invite controls in study-group-detail.html need
accessibility fixes: the `#invite-username` field should have an explicit
accessible label instead of relying on placeholder text, and the `#invite-btn`
should declare its button type. Update the invite markup around `#invite-username`
and `#invite-btn` to add a proper label association (or equivalent accessible
labeling) and set the button’s type explicitly so the control semantics are
clear.
In `@public/study-groups.html`:
- Around line 94-110: The error-handling paths in the study groups flows are
assuming every non-ok response is valid JSON, which can throw before the user
sees the real error. Update the `apiFetch` call sites in `create-form`, `join`,
and `leave` so that after an `!res.ok` branch they safely handle non-JSON bodies
(for example by checking the response content type or falling back to
`res.text()`), and only call `res.json()` when parsing is safe. Keep the fix
localized around the existing `apiFetch` usage and the submit/join/leave
handlers so failed responses still surface a usable message.
- Around line 67-68: The search and activity filter inputs are missing
accessible names because they rely only on placeholders. Update the markup for
the `#search` and `#activity-filter` fields in the study groups template to use
proper <label> elements (or equivalent accessible labeling tied to each input),
matching the approach used by the create-form fields. Keep the existing inputs
and ensure each field has a unique, descriptive label that screen readers can
announce.
- Around line 216-232: The join/leave click handlers in the study-groups page
are treating any non-null apiFetch response as success, so failures from the
backend are hidden. Update the handlers around the group-grid and my-groups
listeners to check res.ok before calling loadGroups(), and surface an error
state/message when join or leave returns a non-2xx response. Keep the behavior
localized to the existing event handlers and apiFetch flow so the UI only
refreshes after a confirmed success.
In `@src/study_groups.py`:
- Around line 394-409: The invite creation logic in the study group flow is
treating any UNIQUE conflict as a duplicate pending invite, but the unique
constraint on the study_group_invites row means an existing declined or older
invite should be reused instead. Update the invite handling in the study_groups
invite path to detect the conflict and refresh the existing row back to pending,
while also updating inviter_id and updated_at, rather than returning the
“pending invite already exists” error. Keep the existing helper-based error
handling around the invite creation code, but adjust the conflict branch so
re-inviting the same user/group pair succeeds by reusing the existing invite
record.
In `@src/worker.py`:
- Around line 2804-2808: The fallback loader in worker.py relies on
importlib.util but only importlib is imported, so this can still fail with
AttributeError if util was not loaded elsewhere. Update the import section so
importlib.util is explicitly imported, then keep using the existing
spec_from_file_location/module_from_spec flow in the fallback block around
study_groups loading.
In `@tests/test_study_groups_api.py`:
- Around line 123-141: The duplicate-join assertion in
test_join_public_group_success_and_join_twice_conflict is too permissive because
it accepts both 400 and 409. Update the second request assertion to the single
status code actually returned by the join flow in worker._dispatch for an
already-member case, so the test verifies the exact API contract instead of
allowing regressions.
🪄 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: d5bf1849-3527-496b-b419-804fd365f3a0
⛔ Files ignored due to path filters (1)
migrations/0011_add_study_groups.sqlis excluded by!**/migrations/**
📒 Files selected for processing (8)
public/invitations.htmlpublic/partials/navbar.htmlpublic/study-group-detail.htmlpublic/study-groups.htmlschema.sqlsrc/study_groups.pysrc/worker.pytests/test_study_groups_api.py
|
please update this to use the activity object as type study group |
|
@A1L13N This means _create_group will insert its own activities row (type=study_group, host_id=creator, title=name) directly, rather than accepting a pre-existing activity_id from the client right? |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
public/invitations.html (1)
87-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth branches of
respond()now do the same thing — andarticleis unused. ✨Nice work removing the stale fade-out! One leftover: after the network call, success and failure paths both
await loadInvites(), so theif (!res || !res.ok)early return is a no-op and the optimisticsplice+render()is always overwritten by the reload. Thearticleparameter is also no longer read. You can collapse this to a singleawait loadInvites().This overlaps the earlier note about the redundant success-path
loadInvites().♻️ Suggested simplification
- async function respond(inviteId, action, article) { + async function respond(inviteId, action) { const idx = invites.findIndex(i => i.id === inviteId); if (idx !== -1) invites.splice(idx, 1); render(); - const res = await apiFetch(`/api/invitations/${encodeURIComponent(inviteId)}/respond`, { + await apiFetch(`/api/invitations/${encodeURIComponent(inviteId)}/respond`, { method: 'POST', body: JSON.stringify({ action }), }); - - if (!res || !res.ok) { - await loadInvites(); - return; - } await loadInvites(); }Note: update the caller at Line 111 to
await respond(inviteId, action)if you drop the parameter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/invitations.html` around lines 87 - 102, The respond() flow in invitations handling has redundant success/failure branches and an unused article parameter. In the respond invite handler, collapse the post-request logic so both outcomes share a single await loadInvites() path, and remove the no-op conditional early return; if article is no longer needed, drop it from respond() and update the caller that invokes respond(inviteId, action, article) to pass only the needed arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/study_groups.py`:
- Around line 288-298: The study group delete flow in the delete handler should
remove both the study_groups record and its backing activities row. Update the
logic around the existing creator check and DELETE in the study group deletion
path to also delete the matching activity entry for the same group_id, and do
both operations in the same transaction so they stay consistent. Use the
existing delete handler and the group creation/activity linkage to locate the
related activity record.
---
Duplicate comments:
In `@public/invitations.html`:
- Around line 87-102: The respond() flow in invitations handling has redundant
success/failure branches and an unused article parameter. In the respond invite
handler, collapse the post-request logic so both outcomes share a single await
loadInvites() path, and remove the no-op conditional early return; if article is
no longer needed, drop it from respond() and update the caller that invokes
respond(inviteId, action, article) to pass only the needed arguments.
🪄 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: 3ae37dc6-4349-4d04-8f71-b5efddb9799e
⛔ Files ignored due to path filters (1)
migrations/0011_add_study_groups.sqlis excluded by!**/migrations/**
📒 Files selected for processing (6)
public/invitations.htmlpublic/study-group-detail.htmlschema.sqlsrc/study_groups.pysrc/worker.pytests/test_study_groups_api.py
💤 Files with no reviewable changes (1)
- src/worker.py
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@schema.sql`:
- Around line 30-31: The study_group schema currently relies on comments for
invariants, which allows invalid rows such as a NULL max_members and can break
capacity checks in study_groups.py. Update the study_group table definition to
add DB CHECK constraints for max_members, role, and status so the allowed values
are enforced at the database level, and make the same constraint changes in the
worker DDL that creates the table. Use the existing study_group table definition
as the source of truth and ensure max_members cannot be NULL if the backend
expects to compare it numerically.
In `@src/study_groups.py`:
- Around line 114-116: The exception logging in study_groups._list_groups.bind
currently prints bind_args and the raw exception, which can leak user IDs and
group content; update this error handling to keep only stable context like the
operation name and exception type. Remove any request-payload fields from the
print output, and apply the same redaction approach to the other affected
exception handler in the study_groups flow as well.
- Around line 5-670: The new private helpers and request handlers in
study_groups need explicit return type annotations to satisfy Ruff and the
project’s Python typing rules. Add return annotations to functions like
_required_text, _optional_text, _auth, _notify, _run_batch, _lookup_user,
_list_groups, _create_group, _group_detail, _delete_group, _join_group,
_leave_group, _invite_member, _list_invitations, _respond_invitation,
_activity_groups, and handle so their signatures clearly state what they return.
- Around line 339-347: The membership capacity check in the study group join
flow is non-atomic because it counts existing rows before inserting, which can
let concurrent requests overfill the group. Update the join/accept logic around
env.DB.prepare for the study_group_members insert so the max_members predicate
is enforced at insert time within the same transaction/query, rather than
relying on count_row. After attempting the insert, verify exactly one row was
added before proceeding to update invite state; apply the same fix in both
affected join paths.
- Around line 443-463: The UNIQUE-conflict path in study_group_invites handling
returns early after resetting an existing invite to pending, so re-invites never
reach _notify. Update the exception branch in study_groups.py around the invite
creation logic to fall through to the notification step after the successful
UPDATE, using the same invitee/group/user context, and only return the error
response when the update itself fails.
- Around line 147-177: The study-group creation flow in _create_group writes
activities.description directly, but that column is expected to be encrypted at
rest. Update the study-groups handler to accept and use the encryption helper
alongside decrypt_aes, encrypt the incoming description before binding it in the
INSERT for activities, and keep decrypting descriptions in the list/detail
response paths so callers still receive plain text.
In `@src/worker.py`:
- Around line 6379-6389: The import fallback in the study_groups loading block
is too broad because the nested importlib.import_module attempts catch every
Exception and can hide real failures inside study_groups.py. Update the import
logic to catch ModuleNotFoundError only, using the study_groups import attempts
in this block, and only fall back to the next loader when the missing module
matches the one being probed; otherwise re-raise the error so genuine
startup/import-time failures are not masked.
🪄 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: ba10a10f-cd28-495d-9ace-71ef0782fdc5
⛔ Files ignored due to path filters (1)
migrations/0012_refactor_study_groups_to_activities.sqlis excluded by!**/migrations/**
📒 Files selected for processing (4)
public/teach.htmlschema.sqlsrc/study_groups.pysrc/worker.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/study_groups.py`:
- Around line 568-594: The invite acceptance flow in the study group handler is
not atomic because the membership insert succeeds before the invite status
update can fail, leaving a user as a member while the invite stays pending.
Update the accept path in the invitation-handling logic so the conditional
insert and the status change are performed together in one transactional/batched
operation, or add rollback/compensation if the second step fails. Use the
existing acceptance code around the membership insert, joined lookup, and invite
update to keep the state of study_group_members and study_group_invites
consistent.
- Around line 120-125: The comment inside study_groups.py is over-indented and
triggers Flake8 E114/E117; adjust the indentation in the loop around the
decrypt_aes call in the relevant study group response-building block so the
inline comment aligns with the surrounding code in the same scope. Use the
nearby symbols enc, helpers["decrypt_aes"], and groups.append to locate and
normalize the indentation without changing logic.
- Line 2: Remove the unused typing imports from the module import list in
study_groups.py; specifically clean up the top-level typing import that
currently brings in Awaitable, Callable, and List, since they are not referenced
anywhere and trigger Flake8 F401. Keep only the type names actually used by the
functions and classes in this file, and verify the remaining imports still
satisfy the type hints in the module.
🪄 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: 3c4fdfd6-bb81-4ba3-977d-b912455b94e6
⛔ Files ignored due to path filters (1)
migrations/0012_refactor_study_groups_to_activities.sqlis excluded by!**/migrations/**
📒 Files selected for processing (3)
schema.sqlsrc/study_groups.pysrc/worker.py
|
@coderabbitai please review make sure naming convention is flexible and make sure we’re not duplicating similar data structures |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd -a 'schema.sql' | head
echo "---schema study group tables---"
rg -n 'study_group|activity_participants|CREATE TABLE' schema.sql | head -80
echo "---study_groups.py outline---"
fd study_groups.py
ast-grep outline src/study_groups.py --items all | head -150
echo "---worker.py study group refs---"
rg -n 'study_group|activities' src/worker.py | head -100
echo "---check for existing similar participant tables---"
rg -n 'CREATE TABLE' schema.sqlLength of output: 11179 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '90,135p' schema.sql
echo "---worker.py activity_members / activity_invites usage---"
rg -n 'activity_members|activity_invites' src/worker.py | head -60
echo "---worker.py study_group_members / study_group_invites definitions---"
sed -n '880,1000p' src/worker.py
echo "---migrations dir---"
fd -a . migrations
cat migrations/0004_add_study_groups.sql 2>/dev/null | head -100Length of output: 9421 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n 'study_group_members|study_group_invites|activity_members|activity_invites|INSERT INTO activities' src/study_groups.py
echo "---migrations file search---"
fd -a '0004' .
fd -a 'add_study_groups' .Length of output: 2581 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '320,420p' src/study_groups.py
echo "-----"
sed -n '440,480p' src/study_groups.py
echo "-----"
sed -n '555,600p' src/study_groups.pyLength of output: 7696
However, the rename inside
Since Suggested fix: replace all remaining Want me to open a follow-up fix for this, or would you like it addressed directly in this PR? ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/study_groups.py (1)
452-470: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winReplace the remaining legacy table names in invite flows.
The schema and worker DDL now define
activity_invitesandactivity_members;study_group_invitesandstudy_group_membersno longer exist. As written, invite creation and invite acceptance will fail at runtime.🐛 Proposed fix
- "SELECT id FROM study_group_invites" + "SELECT id FROM activity_invites" @@ - "INSERT INTO study_group_invites (id,activity_id,inviter_id,invitee_id,status)" + "INSERT INTO activity_invites (id,activity_id,inviter_id,invitee_id,status)" @@ - "UPDATE study_group_invites" + "UPDATE activity_invites" @@ - " WHERE (SELECT COUNT(*) FROM study_group_members WHERE activity_id=?)" + " WHERE (SELECT COUNT(*) FROM activity_members WHERE activity_id=?)"Also applies to: 572-575
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/study_groups.py` around lines 452 - 470, The invite flow still uses removed legacy tables, so update the study group invite creation and acceptance logic to read/write the new schema names. Replace every remaining use of study_group_invites and study_group_members in the invite flow helpers/handlers with activity_invites and activity_members, including the pending-invite lookup, insert/update retry path, and acceptance/member-creation queries, so the runtime queries match the worker DDL.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/study_groups.py`:
- Around line 452-470: The invite flow still uses removed legacy tables, so
update the study group invite creation and acceptance logic to read/write the
new schema names. Replace every remaining use of study_group_invites and
study_group_members in the invite flow helpers/handlers with activity_invites
and activity_members, including the pending-invite lookup, insert/update retry
path, and acceptance/member-creation queries, so the runtime queries match the
worker DDL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8b9492c2-4b90-49bc-a35f-86cbddae22df
⛔ Files ignored due to path filters (1)
migrations/0012_refactor_study_groups_to_activities.sqlis excluded by!**/migrations/**
📒 Files selected for processing (3)
schema.sqlsrc/study_groups.pysrc/worker.py
|
|
||
| if path.startswith("/api/"): | ||
| if ( | ||
| path.startswith("/api/study-groups") |
There was a problem hiding this comment.
The api should be /activity?type=study_group or something like that
|
Closing this PR as it still contains a number of outdated changes from before the rebase along with the current implementation, making the diff confusing. I have raised a new updated PR with clean changes. |
Adds study group creation, membership, and invitation management to the Cloudflare Workers platform.
Database (
migrations/0004_add_study_groups.sql,schema.sql)Three new tables:
study_groups,study_group_members,study_group_invites. Invite status usespending | accepted | declinedmatching Django source.activity_idis nullable to allow standalone groups. Unique constraints on(group_id, user_id)and(group_id, invitee_id)enforced at DB level.Backend (
src/study_groups.py,src/worker.py)Ten endpoints covering group CRUD, join/leave, invite and respond flows. Multi-table writes use
env.DB.batch()for atomicity (group create + member insert, invite accept + member insert). Creator leave is blocked with a 400 matching Django behavior. Module lazy-loaded inside matched route branch only. Notifications wired for join, invite, accept, and decline events.Frontend (
public/study-groups.html,public/study-group-detail.html,public/invitations.html)Group listing with search, create form, join/leave actions, detail view with member list and invite flow, and invitations page with accept/decline. Private group 403 shown as explicit UI message. Auth redirect and Bearer token fetch pattern matches existing pages.
Navigation (
public/partials/navbar.html)Study Groups added to desktop and mobile LEARN sections.
Tests (
tests/test_study_groups_api.py)45 tests covering auth, group lifecycle, membership rules, invitation flows, creator-leave block, duplicate invite 400, and capacity enforcement on accept.
Adds first-class “study group” functionality by representing study groups as
activitiesrows (type='study_group') and introducing activity-generic membership/invitation storage.Key changes:
Database/schema
activitieswithmax_members(nullable;>= 2when set),is_private(default0), andupdated_at.activity_members((activity_id, user_id)unique;role=creator|member) andactivity_invites((activity_id, invitee_id)unique;status=pending|accepted|declined), plus supporting indexes.migrations/0012_refactor_study_groups_to_activities.sqland reflected inschema.sql.Backend
src/study_groups.pywith APIs for listing/creating groups, fetching details with private-group access enforcement, deleting (creator-only), joining/leaving, inviting, and responding to invitations; notifications are sent on join/invite/accept/decline.src/worker.pyvia lazy-loaded module dispatch for/api/study-groups*and/api/invitations*, and for group-related activity routes.activitiesrow (type='study_group',host_id=creator,title=name).Frontend
public/study-groups.htmlto browse study groups (via the activity browser flow usingtype=study_groupand activity cards).public/teach.htmlto allow creating activities of typestudy_group.study_groupdisplay support in existing activity UI templates (icons/labels).Potential impact:
max_membersshould be enforced on joins/acceptance.src/study_groups.pystill contains several SQL references to legacy table names (study_group_members,study_group_invites) in join/invite/capacity-check paths, but the schema defines onlyactivity_membersandactivity_invites, so those paths may fail unless the remaining references are updated.