Activity and Session Editable Functionality#18
Open
Shubhashish-Chakraborty wants to merge 7 commits intoalphaonelabs:mainfrom
Open
Activity and Session Editable Functionality#18Shubhashish-Chakraborty wants to merge 7 commits intoalphaonelabs:mainfrom
Shubhashish-Chakraborty wants to merge 7 commits 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:
WalkthroughFrontend Changes
Sequence DiagramssequenceDiagram
participant User
participant Browser as Browser (teach.html)
participant Server as Server (worker.py)
participant DB as Database
User->>Browser: Click "Edit" on activity
Browser->>Server: GET /api/activities/:id
Server->>DB: Query activity + tags
DB-->>Server: Activity + tags
Server-->>Browser: Activity JSON
Browser->>Browser: Populate form fields, update title/button, show cancel
User->>Browser: Modify activity + tags
User->>Browser: Click "Update Activity"
Browser->>Server: PUT /api/activities/:id with updated fields + tags
Server->>Server: Verify JWT + ownership
Server->>DB: Update activity record
Server->>DB: Delete activity_tags for activity
Server->>DB: Upsert tags & insert activity_tags junctions
DB-->>Server: Success
Server-->>Browser: 200 OK
Browser->>Browser: Show success, reset form state, refresh lists
sequenceDiagram
participant User
participant Browser as Browser (teach.html)
participant Server as Server (worker.py)
participant DB as Database
User->>Browser: Select activity from dropdown
Browser->>Server: GET /api/activities/:actId
Server->>DB: Query activity and sessions
DB-->>Server: Sessions list
Server-->>Browser: Sessions JSON
Browser->>Browser: Store currentActivitySessions, render list with "Edit" buttons
User->>Browser: Click "Edit" on session
Browser->>Browser: Populate session form (format datetime-local), update title/button, show cancel
User->>Browser: Modify session details
User->>Browser: Click "Update Session"
Browser->>Server: PUT /api/sessions/:id with updated fields
Server->>Server: Verify JWT + access
Server->>DB: Update session record (encrypt fields as needed)
DB-->>Server: Success
Server-->>Browser: 200 OK
Browser->>Browser: Show success, reset form
Browser->>Server: GET /api/activities/:actId
Browser->>Browser: Refresh sessions list
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
public/teach.html (1)
266-271: 🧹 Nitpick | 🔵 TrivialThe
isErrparameter inshowMsgis unused.The function accepts an
isErrparameter but ignores it. The color differentiation actually comes from targeting different elements (act-errvsact-ok). Consider removing the unused parameter to avoid confusion, or add a TODO if you plan to use it for styling in the future.🧹 Optional cleanup
- function showMsg(id, msg, isErr) { + function showMsg(id, msg) { const el = document.getElementById(id); el.textContent = msg; el.classList.remove('hidden'); setTimeout(() => el.classList.add('hidden'), 4000); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/teach.html` around lines 266 - 271, The function showMsg currently declares an unused parameter isErr; remove the isErr parameter from showMsg's signature and all call sites (or if you intend to use it later, add a clear TODO comment inside showMsg referencing isErr) so the code no longer contains an unused parameter; update any callers that pass isErr to just pass id and msg (or adjust them to set the appropriate element id like 'act-err' vs 'act-ok') and ensure no lint warnings remain.
🤖 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/teach.html`:
- Around line 367-375: The session ID inserted into the onclick string should be
escaped like other IDs: update the ul.innerHTML mapping that builds the list
(where currentActivitySessions is mapped) to pass esc(s.id) into the
startEditSession(...) call instead of raw s.id; modify the template expression
in the mapping that currently uses startEditSession('${s.id}') to use the esc
helper (same esc used for s.title) so the startEditSession invocation is
consistently defensively escaped.
- Around line 139-164: Each <label> should include a for attribute matching its
associated input/textarea id to improve accessibility; update the labels for the
fields with ids s-title, s-start, s-end, s-location and s-desc so they read
for="s-title", for="s-start", for="s-end", for="s-location" and for="s-desc"
respectively (ensure the textarea's label uses for="s-desc"); no other
structural changes required.
- Around line 358-360: The fetch block that requests '/api/activities/' + actId
currently returns silently on failure (if !res.ok), leaving the UI unchanged;
update this to surface an error to the user like startEditActivity does—on
non-ok response, parse any error payload if available and call the same UI error
handler (e.g., the function used by startEditActivity or a shared
showError/notify method) or display a visible message in the relevant DOM area
so users are informed instead of silently returning; ensure you still bail out
after showing the error and keep references to actId, token, res, and data
unchanged.
In `@src/worker.py`:
- Around line 1214-1223: The code currently proceeds to the DB update flow and
returns ok(None, "Session updated") even when the updates list is empty; modify
the api_update_session.update_session logic to short-circuit when updates is
empty: before appending ses_id and building query, check if not updates and
return ok(None, "No changes" or a similar no-op message) so consumers can
distinguish a successful no-op from an actual update; keep the existing DB
prepare/bind/run and exception handling (env.DB.prepare, params, try/except)
unchanged for the real update path.
- Around line 1132-1155: Replace the current non-atomic delete-and-reinsert
logic for tags (uses env.DB.prepare with DELETE FROM activity_tags and
subsequent INSERT/INSERT OR IGNORE into activity_tags and tags, referencing
act_id and new_id()) with a single database transaction so tag updates are
atomic: begin a transaction, perform the DELETE FROM activity_tags, ensure
INSERT into tags (creating missing tag ids) and INSERT OR IGNORE into
activity_tags all inside that transaction, and commit or rollback on error; also
stop swallowing exceptions — catch exceptions around DB operations and log them
with context via your logger instead of bare `except: pass` so failures are
visible and the transaction can be rolled back on error.
---
Outside diff comments:
In `@public/teach.html`:
- Around line 266-271: The function showMsg currently declares an unused
parameter isErr; remove the isErr parameter from showMsg's signature and all
call sites (or if you intend to use it later, add a clear TODO comment inside
showMsg referencing isErr) so the code no longer contains an unused parameter;
update any callers that pass isErr to just pass id and msg (or adjust them to
set the appropriate element id like 'act-err' vs 'act-ok') and ensure no lint
warnings remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7bc3db26-58cf-492f-8d52-aaa63a7f902e
📒 Files selected for processing (2)
public/teach.htmlsrc/worker.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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)
public/teach.html (1)
60-103:⚠️ Potential issue | 🟡 MinorAdd
forattributes to activity form labels for accessibility.The session form labels were updated to include
forattributes, but the activity form labels are missing them. Screen reader users benefit greatly from programmatic label associations, especially on an educational platform!♿ Proposed fix for label associations
<div> - <label class="block text-sm font-medium text-slate-700 mb-1">Title *</label> + <label for="a-title" class="block text-sm font-medium text-slate-700 mb-1">Title *</label> <input id="a-title" type="text" required </div> <div> - <label class="block text-sm font-medium text-slate-700 mb-1">Description</label> + <label for="a-desc" class="block text-sm font-medium text-slate-700 mb-1">Description</label> <textarea id="a-desc" rows="3" </div> <div class="grid grid-cols-3 gap-3"> <div> - <label class="block text-sm font-medium text-slate-700 mb-1">Type</label> + <label for="a-type" class="block text-sm font-medium text-slate-700 mb-1">Type</label> <select id="a-type" </div> <div> - <label class="block text-sm font-medium text-slate-700 mb-1">Format</label> + <label for="a-format" class="block text-sm font-medium text-slate-700 mb-1">Format</label> <select id="a-format" </div> <div> - <label class="block text-sm font-medium text-slate-700 mb-1">Schedule</label> + <label for="a-schedule" class="block text-sm font-medium text-slate-700 mb-1">Schedule</label> <select id="a-schedule" </div> </div> <div> - <label class="block text-sm font-medium text-slate-700 mb-1">Tags (comma-separated)</label> + <label for="a-tags" class="block text-sm font-medium text-slate-700 mb-1">Tags (comma-separated)</label> <input id="a-tags" type="text" </div>As per coding guidelines: "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/teach.html` around lines 60 - 103, The form labels in the activity section lack for attributes, so screen readers can't programmatically associate labels with inputs; update each label to include for="a-title", for="a-desc", for="a-type", for="a-format", for="a-schedule", and for="a-tags" matching the existing input/textarea/select IDs (a-title, a-desc, a-type, a-format, a-schedule, a-tags) to restore correct label-input associations for accessibility.
♻️ Duplicate comments (2)
public/teach.html (2)
358-360:⚠️ Potential issue | 🟡 MinorSilent failure when fetching activity sessions.
If the API call fails, the function returns silently without informing the user. This leaves the UI in a potentially confusing state where nothing happens after selecting an activity. Consider showing an error message similar to how
startEditActivityhandles failures on line 284.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/teach.html` around lines 358 - 360, When fetching activity sessions (the fetch('/api/activities/' + actId, { headers: { Authorization: 'Bearer ' + token } }) call) do not return silently on failure; detect !res.ok, parse any error body if available, and surface a user-visible error (e.g., call the same error display used by startEditActivity or show an alert/toast) before returning so the UI explains the failure; ensure you reference the same UI update path used after a successful fetch (the code that consumes `data`) so the error path cleans up or disables any pending UI state.
367-375:⚠️ Potential issue | 🟡 MinorEscape
start_timeands.idwhen rendering session list items.While
s.titleis properly escaped withesc(), boths.start_timeands.idare inserted directly into the HTML. Although these values come from your backend, it's good defensive practice to escape all dynamic content—this protects against unexpected data issues and maintains consistency with how other values are handled.🛡️ Proposed defensive escaping
ul.innerHTML = currentActivitySessions.map(s => { return `<li class="px-4 py-2 flex justify-between items-center bg-white hover:bg-slate-50"> <div> <span class="font-semibold text-slate-800">${esc(s.title)}</span> - <div class="text-xs text-slate-500">${s.start_time || ''}</div> + <div class="text-xs text-slate-500">${esc(s.start_time || '')}</div> </div> - <button type="button" onclick="startEditSession('${s.id}')" class="text-xs text-brand font-semibold border border-brand/30 px-2 py-1 rounded hover:bg-indigo-50">Edit</button> + <button type="button" onclick="startEditSession('${esc(s.id)}')" class="text-xs text-brand font-semibold border border-brand/30 px-2 py-1 rounded hover:bg-indigo-50">Edit</button> </li>`; }).join('');As per coding guidelines: "Review HTML templates for... XSS risks from unescaped user content".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/teach.html` around lines 367 - 375, The session list rendering inserts unescaped s.start_time and s.id into HTML; update the currentActivitySessions.map block that sets ul.innerHTML to escape both values (use esc(s.start_time) for the displayed time and esc(s.id) for any injected id), and remove the direct interpolation into the onclick string — instead render a data-session-id attribute (e.g., data-session-id="${esc(s.id)}") on the button and attach click handlers that call startEditSession(button.dataset.sessionId) (or use esc(s.id) if you must keep inline onclick), so all dynamic values are consistently escaped and startEditSession is invoked safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@public/teach.html`:
- Around line 60-103: The form labels in the activity section lack for
attributes, so screen readers can't programmatically associate labels with
inputs; update each label to include for="a-title", for="a-desc", for="a-type",
for="a-format", for="a-schedule", and for="a-tags" matching the existing
input/textarea/select IDs (a-title, a-desc, a-type, a-format, a-schedule,
a-tags) to restore correct label-input associations for accessibility.
---
Duplicate comments:
In `@public/teach.html`:
- Around line 358-360: When fetching activity sessions (the
fetch('/api/activities/' + actId, { headers: { Authorization: 'Bearer ' + token
} }) call) do not return silently on failure; detect !res.ok, parse any error
body if available, and surface a user-visible error (e.g., call the same error
display used by startEditActivity or show an alert/toast) before returning so
the UI explains the failure; ensure you reference the same UI update path used
after a successful fetch (the code that consumes `data`) so the error path
cleans up or disables any pending UI state.
- Around line 367-375: The session list rendering inserts unescaped s.start_time
and s.id into HTML; update the currentActivitySessions.map block that sets
ul.innerHTML to escape both values (use esc(s.start_time) for the displayed time
and esc(s.id) for any injected id), and remove the direct interpolation into the
onclick string — instead render a data-session-id attribute (e.g.,
data-session-id="${esc(s.id)}") on the button and attach click handlers that
call startEditSession(button.dataset.sessionId) (or use esc(s.id) if you must
keep inline onclick), so all dynamic values are consistently escaped and
startEditSession is invoked safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0f4dfb42-e975-4307-a371-33e3c658f3b1
📒 Files selected for processing (1)
public/teach.html
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/teach.html`:
- Around line 371-379: The start_time value in the ul.innerHTML build (iterating
currentActivitySessions) is inserted unescaped; update the template to pass
s.start_time through the same esc(...) sanitizer used for title and id (e.g.,
use esc(s.start_time || '') where s.start_time is rendered) so the rendered
datetime is HTML-escaped for defense-in-depth; modify the template string inside
the currentActivitySessions.map callback (where ul.innerHTML is assigned) to
call esc on s.start_time.
In `@src/worker.py`:
- Around line 1131-1134: The early return "return ok(None, 'No changes
provided')" skips tag-only updates; modify the change-detection logic around
that return so that presence of "tags" in the request body is treated as a
change (either move the tag synchronization block before the early return or
update the conditional that decides "no changes" to include body.get('tags')),
ensuring the tag sync code that follows the return (the "if 'tags' in body:"
block) executes for tag-only requests; update any variables or flags used to
detect changes so tag updates trigger the downstream save/sync logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d60d1131-a6ea-4dc1-9a72-e43eac3fb76e
📒 Files selected for processing (2)
public/teach.htmlsrc/worker.py
Contributor
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/worker.py (1)
1134-1158:⚠️ Potential issue | 🟠 MajorMake the tag replacement all-or-nothing.
This still deletes the existing
activity_tagsrows and then recreates them one by one while only logging failures. If any insert fails after the delete, the activity is left with a partial tag set; if the delete fails, you can end up with a stale union of old and new tags. In both cases the handler still returns"Activity updated". Please wrap the delete/reinsert flow in one transaction and fail the request on rollback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker.py` around lines 1134 - 1158, Wrap the delete-and-reinsert tag flow in a single database transaction so it is atomic: start a transaction via the DB API before calling env.DB.prepare("DELETE FROM activity_tags...") for act_id, perform all tag SELECT/INSERT and activity_tags INSERT OR IGNORE operations (using act_id, tag_name_clean, tag_id, env.DB.prepare) inside that transaction, and then commit; on any exception rollback the transaction, capture the exception (reuse capture_exception with context like "api_update_activity.transaction"), and return/fail the request instead of proceeding to return "Activity updated". Ensure you do not delete tags outside the transaction and that rollback is triggered on any insert/select error so partial tag sets cannot remain.
🤖 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/teach.html`:
- Around line 129-136: The section titles "Existing Sessions" and "New Session
Details" are using <label> (IDs: existing-sessions, ses-fields-title) which is
semantically incorrect; replace those <label> elements with appropriate heading
elements (e.g., <h3> or <h2>) inside the containers (ses-fields and
existing-sessions) and keep the visual classes for styling, and then associate
the session list (s-session-list) with its heading via
aria-labelledby="ses-fields-title" (or a new heading id) so assistive tech
recognizes the list as belonging to that section.
- Around line 273-275: Add a per-request guard (a simple incrementing request
token or an AbortController) to the activity/session loader logic so late fetch
results cannot overwrite newer UI state: when starting a new fetch for the edit
form, create a new token/controller, store it in a module-scoped variable,
cancel or bump the token for any previous request, and after each await verify
the token is still current (or that the controller was not aborted) before
mutating editActivityId, editSessionId, or currentActivitySessions; apply the
same pattern to both loaders referenced around the existing edit code so only
the latest response updates the form/list.
In `@src/worker.py`:
- Around line 1089-1121: The update branches assume inputs are strings and call
.strip() directly, causing AttributeError for non-string payloads; before
calling .strip() on title, description, atype, fmt, and schedule_type validate
with isinstance(value, str) and return a 400 via err("FIELD must be a string")
(use the same err(...) helper) for bad types, then proceed to strip and the
existing validation; for description ensure you only call encrypt(description,
env.ENCRYPTION_KEY) when description is a non-empty string; apply the same
isinstance checks to the other affected block referenced (the similar branches
around the 1188-1215 region) so non-string inputs return a controlled 400
instead of raising.
---
Duplicate comments:
In `@src/worker.py`:
- Around line 1134-1158: Wrap the delete-and-reinsert tag flow in a single
database transaction so it is atomic: start a transaction via the DB API before
calling env.DB.prepare("DELETE FROM activity_tags...") for act_id, perform all
tag SELECT/INSERT and activity_tags INSERT OR IGNORE operations (using act_id,
tag_name_clean, tag_id, env.DB.prepare) inside that transaction, and then
commit; on any exception rollback the transaction, capture the exception (reuse
capture_exception with context like "api_update_activity.transaction"), and
return/fail the request instead of proceeding to return "Activity updated".
Ensure you do not delete tags outside the transaction and that rollback is
triggered on any insert/select error so partial tag sets cannot remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1cad448a-e4ba-45e4-aa3c-3cef3b76a8ba
📒 Files selected for processing (2)
public/teach.htmlsrc/worker.py
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/worker.py (1)
1146-1170:⚠️ Potential issue | 🟠 MajorTag replacement can still commit a partial result.
This path deletes all existing links, logs insert failures, and then still returns
"Activity updated". If one insert fails, the activity is left with only a subset of its tags. The delete-and-reinsert sequence needs to be atomic, and the request should fail if the replacement cannot be completed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker.py` around lines 1146 - 1170, The delete-then-reinsert of activity tags in api_update_activity is not atomic: you DELETE existing activity_tags then try INSERTs and still return success even if some INSERTs fail, leaving a partial state; wrap the delete-and-reinsert sequence in a database transaction (BEGIN/COMMIT) and on any exception during inserts rollback the transaction and surface a failure response (log via capture_exception as currently used), so use the DB transaction API around the block that runs DELETE FROM activity_tags and the subsequent INSERT OR IGNORE INTO activity_tags (referencing act_id, tags, new_id(), and the error log keys like "api_update_activity.insert_tag" and "api_update_activity.insert_activity_tag") to ensure either all tags are replaced or the operation is aborted and rolled back.public/teach.html (1)
279-288:⚠️ Potential issue | 🟠 MajorGuard loader failures before mutating the edit UI.
These loaders still do async work outside a guarded
try/catch, andonActivitySelect()only checksloadSeqon the happy path. A rejectedfetch, a non-JSON error body, or a stale non-OK response can still leave the user with the wrong error state or a broken edit action. Please gate all UI mutations — success and error — behind the active request token.Also applies to: 363-370
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/teach.html` around lines 279 - 288, startEditActivity's async fetch and JSON parsing mutate the UI outside a guarded try/catch and only check activityEditLoadSeq on the happy path; wrap the network and parse steps in a try/catch, check loadSeq (the local loadSeq captured from activityEditLoadSeq) immediately after each await and before any UI mutation (including error paths that call showMsg), and in the catch handler also verify loadSeq before calling showMsg or updating the edit UI; apply the same pattern to the similar block referenced around onActivitySelect / lines 363-370 so every success and failure path is gated by the active request token.
🤖 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/teach.html`:
- Around line 344-346: Before calling loadHostedActivities() from the success
paths (the block that calls showMsg/cancelEditActivity and the other occurrence
around lines 455-458), capture the current selection of the activity dropdown
(`#s-activity`) into a variable (e.g., selectedActivityId), call
loadHostedActivities(), then after it finishes restore the dropdown selection by
setting `#s-activity.value` back to selectedActivityId and trigger the same
change/selection logic that updates the session editor; ensure you do this for
both call sites so the visible session list remains anchored to the previously
selected activity.
In `@src/worker.py`:
- Around line 1106-1131: The current update logic silently defaults invalid enum
inputs (variables atype, fmt, schedule_type) to valid strings, causing
accidental overwrites; change each block so that after trimming you validate
membership against the allowed tuples and if the value is not in the allowed set
you return err("... must be one of: ...", 400) (use the same err(...) function)
instead of assigning a default, and only append to updates and params when the
value passes validation; update the messages for atype, fmt, and schedule_type
to list the allowed values and ensure you still check types with isinstance(...)
before validation.
- Around line 1144-1156: The handler currently does tags = body.get("tags") or
[] and then iterates, which treats strings as iterables (e.g., "python" ->
characters); update the input validation so that before iterating you explicitly
check that body["tags"] is a list (or tuple) and that every element is a string:
if not a sequence, raise/return a validation error; if it is, coerce to list and
reject non-string or empty-string entries early. Locate the code around body,
tags, act_id and the DB call env.DB.prepare("DELETE FROM activity_tags WHERE
activity_id=?") and add the validation logic instead of silently coercing,
ensuring you still call capture_exception(...) on DB errors as currently done.
---
Duplicate comments:
In `@public/teach.html`:
- Around line 279-288: startEditActivity's async fetch and JSON parsing mutate
the UI outside a guarded try/catch and only check activityEditLoadSeq on the
happy path; wrap the network and parse steps in a try/catch, check loadSeq (the
local loadSeq captured from activityEditLoadSeq) immediately after each await
and before any UI mutation (including error paths that call showMsg), and in the
catch handler also verify loadSeq before calling showMsg or updating the edit
UI; apply the same pattern to the similar block referenced around
onActivitySelect / lines 363-370 so every success and failure path is gated by
the active request token.
In `@src/worker.py`:
- Around line 1146-1170: The delete-then-reinsert of activity tags in
api_update_activity is not atomic: you DELETE existing activity_tags then try
INSERTs and still return success even if some INSERTs fail, leaving a partial
state; wrap the delete-and-reinsert sequence in a database transaction
(BEGIN/COMMIT) and on any exception during inserts rollback the transaction and
surface a failure response (log via capture_exception as currently used), so use
the DB transaction API around the block that runs DELETE FROM activity_tags and
the subsequent INSERT OR IGNORE INTO activity_tags (referencing act_id, tags,
new_id(), and the error log keys like "api_update_activity.insert_tag" and
"api_update_activity.insert_activity_tag") to ensure either all tags are
replaced or the operation is aborted and rolled back.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45ffd1f4-74c4-4943-9670-3f3b33437058
📒 Files selected for processing (2)
public/teach.htmlsrc/worker.py
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (3)
public/teach.html (2)
344-346:⚠️ Potential issue | 🟠 MajorPreserve the selected activity after refreshing the hosted list.
loadHostedActivities()rebuilds#s-activity, so this success path clears the current selection while the visible session list still reflects the previously selected activity. Capture the current value before reloading, restore it afterward, and callonActivitySelect()so the dropdown and session list stay aligned.💡 One simple fix
- showMsg('act-ok', `Activity ${editActivityId ? 'updated' : 'created'} successfully!`, false); - cancelEditActivity(); - await loadHostedActivities(); + const selectedActivityId = document.getElementById('s-activity').value; + showMsg('act-ok', `Activity ${editActivityId ? 'updated' : 'created'} successfully!`, false); + cancelEditActivity(); + await loadHostedActivities(); + if (selectedActivityId) { + document.getElementById('s-activity').value = selectedActivityId; + } + await onActivitySelect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/teach.html` around lines 344 - 346, Before calling loadHostedActivities(), capture the current selected activity value from the `#s-activity` select (or the editActivityId) into a temporary variable, then call loadHostedActivities(), and after it completes restore the select's value to that captured value and invoke onActivitySelect() so the dropdown and session list remain aligned; update the success path that calls showMsg(...), cancelEditActivity(), and await loadHostedActivities() to save/restore selection and call onActivitySelect() afterward (use symbols editActivityId, loadHostedActivities, onActivitySelect, and the `#s-activity` element to locate the change).
123-125:⚠️ Potential issue | 🟡 MinorLink the activity label to the select.
This control is still missing a programmatic label, so assistive tech will not announce “Activity” when
#s-activityreceives focus.♿ Small accessibility fix
- <label class="block text-sm font-medium text-slate-700 mb-1">Activity *</label> + <label for="s-activity" class="block text-sm font-medium text-slate-700 mb-1">Activity *</label>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/teach.html` around lines 123 - 125, The label element for the Activity control is not programmatically associated with the select#s-activity, so screen readers won't announce it; update the label to reference the select by adding a matching for="s-activity" attribute (or alternatively wrap the select in the <label>) so the label and the select#s-activity are linked (ensure the select keeps id="s-activity" and that onchange="onActivitySelect()" remains).src/worker.py (1)
1133-1174:⚠️ Potential issue | 🟠 MajorMake this update path all-or-nothing.
With a payload like
{"title":"New title","tags":"python"}, Lines 1133-1140 persist the activity change before Lines 1144-1149 reject the request with400. Even after validation passes, failures in Lines 1150-1174 are only logged and the handler still returns200, so callers can get partial updates with the wrong status. Please validatetagsbefore the first write and execute the activity row update plus tag replacement as a single transaction/batch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker.py` around lines 1133 - 1174, The current flow updates the activities row before validating tags and performs tag deletes/inserts without transaction, leading to partial updates and incorrect 200 responses; first validate body.get("tags") (ensure it's None or a list of strings) before any DB write, then wrap the activities UPDATE, DELETE FROM activity_tags and subsequent INSERTs (use the same act_id, new_id logic and queries: "UPDATE activities ...", "DELETE FROM activity_tags ...", "INSERT INTO tags ...", "INSERT OR IGNORE INTO activity_tags ...") inside a single database transaction so any exception causes rollback; on any DB error capture_exception(...) and return err(...,500) (do not just log and continue), and ensure validation errors return 400 before starting the transaction.
🤖 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/teach.html`:
- Around line 344-346: Before calling loadHostedActivities(), capture the
current selected activity value from the `#s-activity` select (or the
editActivityId) into a temporary variable, then call loadHostedActivities(), and
after it completes restore the select's value to that captured value and invoke
onActivitySelect() so the dropdown and session list remain aligned; update the
success path that calls showMsg(...), cancelEditActivity(), and await
loadHostedActivities() to save/restore selection and call onActivitySelect()
afterward (use symbols editActivityId, loadHostedActivities, onActivitySelect,
and the `#s-activity` element to locate the change).
- Around line 123-125: The label element for the Activity control is not
programmatically associated with the select#s-activity, so screen readers won't
announce it; update the label to reference the select by adding a matching
for="s-activity" attribute (or alternatively wrap the select in the <label>) so
the label and the select#s-activity are linked (ensure the select keeps
id="s-activity" and that onchange="onActivitySelect()" remains).
In `@src/worker.py`:
- Around line 1133-1174: The current flow updates the activities row before
validating tags and performs tag deletes/inserts without transaction, leading to
partial updates and incorrect 200 responses; first validate body.get("tags")
(ensure it's None or a list of strings) before any DB write, then wrap the
activities UPDATE, DELETE FROM activity_tags and subsequent INSERTs (use the
same act_id, new_id logic and queries: "UPDATE activities ...", "DELETE FROM
activity_tags ...", "INSERT INTO tags ...", "INSERT OR IGNORE INTO activity_tags
...") inside a single database transaction so any exception causes rollback; on
any DB error capture_exception(...) and return err(...,500) (do not just log and
continue), and ensure validation errors return 400 before starting the
transaction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 10a5c73f-0b9d-4aa6-91ee-55acc3af8469
📒 Files selected for processing (2)
public/teach.htmlsrc/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.
The goal of this PR is to allow activities and sessions to be editable after creation. Currently, the user can create them but not update them.
Added Endpoints: PUT
/api/activities/:idand PUT/api/sessions/:idroutes in the backend.Added functions:
api_update_activity(),api_update_session()Updated Frontend:
teach.htmlAfter results:
Do watch the ScreenRecording & see the implementation in action:
activitiesEditableLearn.mp4
Overview
Adds in-place edit/update support for activities and sessions. Backend: new PUT APIs and routing for updating activities and sessions with ownership checks. Frontend: teach.html updated to support start/cancel edit flows and switch forms between create/add and update modes.
Key changes
Backend (src/worker.py)
Frontend (public/teach.html)
Impact