feat(learn): make tutor session names editable#93
Merged
Conversation
Adds PATCH /api/learn/sessions/{id} to rename a session's topic
(handles in-memory pending sessions too) and an inline-edit affordance
in the recent-sessions list with optimistic update + revert on error.
Closes #91
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds session renaming: backend model and PATCH endpoint; frontend API helper, inline rename UI in Learn with optimistic updates and rollback; tests for rename flows. Also introduces a deterministic paletteFor color utility and adjusts graph node color/radius usages. ChangesSession Renaming Feature
Deterministic Color Palette & Graph UI Tweaks
Sequence DiagramsequenceDiagram
participant User as User
participant UI as Learn Component
participant API as Frontend API
participant Server as Backend API
participant DB as Database
User->>UI: Edit session topic, press Enter
UI->>API: renameSession(sessionId, userId, newTopic)
API->>Server: PATCH /api/learn/sessions/{sessionId} {user_id, topic}
alt pending session
Server->>Server: Update in-memory PENDING_SESSIONS payload
Server-->>API: { updated: true, session }
else persisted session
Server->>Server: Validate ownership
Server->>DB: Update session topic
DB-->>Server: Confirm update
Server-->>API: { updated: true, session }
end
API-->>UI: Response with updated session
UI->>UI: Commit optimistic update or rollback on failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
frontend | 8824a6e | Commit Preview URL Branch Preview URL |
May 06 2026, 02:09 AM |
4 tasks
SVG presentation attributes (fill=) don't resolve CSS custom properties, so a missing per-course color fell through to var(--c-sage) and rendered as black. Adds a deterministic hex palette + paletteFor(seed) helper and wires it into the Dashboard, Tree, and KnowledgeGraph fallbacks. Also bumps subject-root radius from 22 to 28 so course family centers stand out from concept nodes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
backend/tests/test_learn_routes.py (1)
286-374: ⚡ Quick winCoverage gap: pending-session 403 path is untested.
The endpoint has a separate ownership branch for pending sessions (
routes/learn.pylines 471–472) that returns 403 whenpending["user_id"] != body.user_id. The current suite covers the 403 path only for persisted sessions; consider adding a pending-session counterpart so that branch is exercised.A test for the 120-char boundary (exactly 120 should succeed) would also be a cheap addition.
🧪 Proposed additional tests
def test_pending_wrong_user_returns_403(self): from routes.learn import PENDING_SESSIONS PENDING_SESSIONS["pending-2"] = { "user_id": "owner", "topic": "Old", "mode": "socratic", "assistant_reply": "hi", } try: r = client.patch( "/api/learn/sessions/pending-2", json={"user_id": "intruder", "topic": "Renamed"}, ) assert r.status_code == 403 assert PENDING_SESSIONS["pending-2"]["topic"] == "Old" finally: PENDING_SESSIONS.pop("pending-2", None) def test_topic_at_120_char_boundary_succeeds(self): sessions_mock = MagicMock() sessions_mock.select.return_value = [{"user_id": "u1"}] def factory(name): if name == "sessions": return sessions_mock m = MagicMock() m.select.return_value = [] return m with patch("routes.learn.table", side_effect=factory): r = client.patch( "/api/learn/sessions/s1", json={"user_id": "u1", "topic": "x" * 120}, ) assert r.status_code == 200🤖 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 `@backend/tests/test_learn_routes.py` around lines 286 - 374, Add two tests under TestRenameSession: one named test_pending_wrong_user_returns_403 that sets routes.learn.PENDING_SESSIONS["pending-2"] = {... "user_id": "owner"...}, issues client.patch to "/api/learn/sessions/pending-2" with body user_id "intruder" and asserts 403 and that PENDING_SESSIONS["pending-2"]["topic"] remains unchanged, cleaning up with PENDING_SESSIONS.pop in a finally block; and one named test_topic_at_120_char_boundary_succeeds that mocks routes.learn.table like the other persisted-session tests (use sessions_mock.select.return_value = [{"user_id":"u1"}] and the factory used elsewhere), calls client.patch with topic = "x"*120 and asserts status_code == 200. Ensure tests reference PENDING_SESSIONS, client.patch, and the same mocking pattern used in existing TestRenameSession tests.frontend/src/components/screens/Learn.tsx (2)
815-832: 💤 Low valueNit: dead
disabled={editing}on the resume button.Lines 816–831 only render this button in the
!editingbranch of the ternary, sodisabled={editing}(line 818) is alwaysfalseat render time. Safe to drop.🤖 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 `@frontend/src/components/screens/Learn.tsx` around lines 815 - 832, The resume button rendered in the !editing branch includes a redundant disabled={editing} prop that is always false; remove the disabled prop from the button JSX in the Learn component (the element that calls onResume(s)) so the button is not passed a dead prop and relies on the surrounding conditional instead.
783-814: ⚡ Quick winAdd an accessible label to the rename input.
When the pencil button is activated, focus moves to a bare
<input>with noaria-labelor associated<label>. Screen-reader users lose the context that this is the session topic field. The pencil button itself hasaria-label="Rename session"but that label is lost once the button is replaced by the input.♿ Proposed fix
<input autoFocus value={draft} maxLength={120} + aria-label="Session topic" onChange={e => setDraft(e.target.value)}🤖 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 `@frontend/src/components/screens/Learn.tsx` around lines 783 - 814, The input shown when editing a session topic lacks an accessible name; update the <input> inside the editing branch in Learn.tsx to provide an accessible label (e.g., add aria-label="Session topic" or aria-labelledby pointing to a hidden/visible label element) so screen readers know this is the session/topic rename field; keep existing handlers (onChange -> setDraft, onKeyDown -> commitEdit/cancelEdit, onBlur -> commitEdit) intact and ensure the added label id is unique and referenced so focus and semantics remain correct.
🤖 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.
Nitpick comments:
In `@backend/tests/test_learn_routes.py`:
- Around line 286-374: Add two tests under TestRenameSession: one named
test_pending_wrong_user_returns_403 that sets
routes.learn.PENDING_SESSIONS["pending-2"] = {... "user_id": "owner"...}, issues
client.patch to "/api/learn/sessions/pending-2" with body user_id "intruder" and
asserts 403 and that PENDING_SESSIONS["pending-2"]["topic"] remains unchanged,
cleaning up with PENDING_SESSIONS.pop in a finally block; and one named
test_topic_at_120_char_boundary_succeeds that mocks routes.learn.table like the
other persisted-session tests (use sessions_mock.select.return_value =
[{"user_id":"u1"}] and the factory used elsewhere), calls client.patch with
topic = "x"*120 and asserts status_code == 200. Ensure tests reference
PENDING_SESSIONS, client.patch, and the same mocking pattern used in existing
TestRenameSession tests.
In `@frontend/src/components/screens/Learn.tsx`:
- Around line 815-832: The resume button rendered in the !editing branch
includes a redundant disabled={editing} prop that is always false; remove the
disabled prop from the button JSX in the Learn component (the element that calls
onResume(s)) so the button is not passed a dead prop and relies on the
surrounding conditional instead.
- Around line 783-814: The input shown when editing a session topic lacks an
accessible name; update the <input> inside the editing branch in Learn.tsx to
provide an accessible label (e.g., add aria-label="Session topic" or
aria-labelledby pointing to a hidden/visible label element) so screen readers
know this is the session/topic rename field; keep existing handlers (onChange ->
setDraft, onKeyDown -> commitEdit/cancelEdit, onBlur -> commitEdit) intact and
ensure the added label id is unique and referenced so focus and semantics remain
correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ea29a994-a0b6-4198-89d1-e1ab42dc1756
📒 Files selected for processing (5)
backend/models/__init__.pybackend/routes/learn.pybackend/tests/test_learn_routes.pyfrontend/src/components/screens/Learn.tsxfrontend/src/lib/api.ts
- Esc no longer commits the rename. The blur fired by unmounting the input
was running commitEdit with the typed draft still in its closure; guard
with a cancellingRef so cancel skips the commit path.
- Stale revert on concurrent renames: only revert the optimistic value if
it's still on screen, so a newer rename can't be clobbered by an older
failed one.
- Drop dead `disabled={editing}` on the resume button (only rendered when
editing is false).
- Harden paletteFor against -2^31 hash overflow that could yield an
out-of-range index.
- Remove unused mock setup from the empty/too-long topic tests — the
validation runs before any DB lookup.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- apiToGraphNode now uses paletteFor() for the color fallback to match Dashboard/Tree (var(--c-sage) doesn't resolve in SVG fill=). - Added aria-label="Session name" to the rename input so screen readers announce a meaningful name. - Track per-session confirmed topics in a ref and use that as the revert target. Fixes the edge case where two back-to-back rename failures left the UI stuck on the first-attempt value while the server held the original. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous revert-to-captured-topic approach was racy when two renames overlapped: depending on which call's previousTopic was captured first and which failed, the UI could end up out of sync with the server. Drop the guess and ask the server for truth on failure — getSessions returns the authoritative state, which we use to refresh both recentSessions and the confirmed-topics ref. Also align the initial-load ref seed with the same message_count > 0 filter we apply to recentSessions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/tests/test_learn_routes.py (2)
312-329: 💤 Low value
PENDING_SESSIONSmutation leaks state if the test is interrupted beforefinally.The
finallyblock handles normal exceptions, but if the test process is killed (e.g., by a timeout orSIGKILL) the dict entry"pending-1"will persist in the module-level dict for any subsequently-run test in the same process. Usingpytest's fixture scoping (e.g., anautousefixture that clearsPENDING_SESSIONSafter each test) is safer. This is a minor concern for sequential runs but becomes a real issue withpytest-xdistor future parallelism.🤖 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 `@backend/tests/test_learn_routes.py` around lines 312 - 329, The test test_renames_pending_session mutates the module-level PENDING_SESSIONS dict and relies on a finally to pop the key, which can leak state if the process is killed; change the test to avoid global mutation by isolating or restoring state via a pytest fixture or monkeypatch: create an autouse fixture (or use monkeypatch) that saves a copy of routes.learn.PENDING_SESSIONS before the test and restores/clears it after each test (or temporarily replace PENDING_SESSIONS with a fresh dict for the duration of test_renames_pending_session) so no entries persist across tests.
331-343: ⚡ Quick winValidation tests bypass auth but may silently depend on
require_selfbeing a no-op.
test_empty_topic_returns_400andtest_topic_too_long_returns_400send requests with no auth headers and no table mock. Per the route handler,require_self(body.user_id, request)is called before the topic validation. Ifrequire_selfever starts enforcing auth in the test environment (e.g., when middleware or test config changes), these tests will return 401/403 instead of 400, creating a silent false-pass today.Add a
require_selfmock to pin the intent:🛡️ Proposed fix
def test_empty_topic_returns_400(self): + with patch("routes.learn.require_self"): r = client.patch( "/api/learn/sessions/s1", json={"user_id": "u1", "topic": " "}, ) assert r.status_code == 400 def test_topic_too_long_returns_400(self): + with patch("routes.learn.require_self"): r = client.patch( "/api/learn/sessions/s1", json={"user_id": "u1", "topic": "x" * 121}, ) assert r.status_code == 400🤖 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 `@backend/tests/test_learn_routes.py` around lines 331 - 343, The two tests (test_empty_topic_returns_400 and test_topic_too_long_returns_400) currently omit auth and can break if require_self stops being a no-op; mock require_self in these tests so it always allows the call (e.g., patch or monkeypatch the require_self function used by the learn session PATCH handler to a stub that returns None/does nothing) before making the client.patch request to ensure the route's topic validation is exercised and the response remains 400 regardless of auth middleware changes.frontend/src/components/screens/Tree.tsx (1)
26-42: ⚡ Quick winConsider extracting
apiToGraphNodeto a shared helper.This exact function is now duplicated in
Tree.tsx,Dashboard.tsx, andLearn.tsx. Any future tweak to the color/fallback chain ormastery_tiermapping has to be done in three places (this PR already had to touch all three). A small helper in@/lib/data(or@/lib/api) keeping the same signature would consolidate the logic.🤖 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 `@frontend/src/components/screens/Tree.tsx` around lines 26 - 42, apiToGraphNode is duplicated across Tree.tsx, Dashboard.tsx, and Learn.tsx; extract it into a single shared helper (e.g., export a function apiToGraphNode with the same signature from a new module in `@/lib/data` or `@/lib/api`), move the logic exactly as-is (preserving color fallback chain, mastery_tier mapping, and returned GraphNode shape), update Tree.tsx, Dashboard.tsx, and Learn.tsx to import and use the shared apiToGraphNode, and remove the local copies so future changes are made in one place.
🤖 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 `@frontend/src/components/screens/Learn.tsx`:
- Around line 225-240: The rollback uses a stale previousTopic captured at
submit time; change handleRenameSession so you keep the optimistic
setRecentSessions and call renameSession as-is, but move the lookup of the
revert target into the catch handler by reading
confirmedTopicsRef.current.get(s.id) ?? s.topic (instead of using the
previousTopic captured before the try); on failure use setRecentSessions to
revert only if the session still has the optimistic topic (same conditional) and
keep updating confirmedTopicsRef.current.set(s.id, trimmed) on success as
before.
---
Nitpick comments:
In `@backend/tests/test_learn_routes.py`:
- Around line 312-329: The test test_renames_pending_session mutates the
module-level PENDING_SESSIONS dict and relies on a finally to pop the key, which
can leak state if the process is killed; change the test to avoid global
mutation by isolating or restoring state via a pytest fixture or monkeypatch:
create an autouse fixture (or use monkeypatch) that saves a copy of
routes.learn.PENDING_SESSIONS before the test and restores/clears it after each
test (or temporarily replace PENDING_SESSIONS with a fresh dict for the duration
of test_renames_pending_session) so no entries persist across tests.
- Around line 331-343: The two tests (test_empty_topic_returns_400 and
test_topic_too_long_returns_400) currently omit auth and can break if
require_self stops being a no-op; mock require_self in these tests so it always
allows the call (e.g., patch or monkeypatch the require_self function used by
the learn session PATCH handler to a stub that returns None/does nothing) before
making the client.patch request to ensure the route's topic validation is
exercised and the response remains 400 regardless of auth middleware changes.
In `@frontend/src/components/screens/Tree.tsx`:
- Around line 26-42: apiToGraphNode is duplicated across Tree.tsx,
Dashboard.tsx, and Learn.tsx; extract it into a single shared helper (e.g.,
export a function apiToGraphNode with the same signature from a new module in
`@/lib/data` or `@/lib/api`), move the logic exactly as-is (preserving color
fallback chain, mastery_tier mapping, and returned GraphNode shape), update
Tree.tsx, Dashboard.tsx, and Learn.tsx to import and use the shared
apiToGraphNode, and remove the local copies so future changes are made in one
place.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d36f329-7ce6-474b-969b-fe605bef07c3
📒 Files selected for processing (6)
backend/tests/test_learn_routes.pyfrontend/src/components/KnowledgeGraph.tsxfrontend/src/components/screens/Dashboard.tsxfrontend/src/components/screens/Learn.tsxfrontend/src/components/screens/Tree.tsxfrontend/src/lib/data.ts
…-names # Conflicts: # backend/tests/test_learn_routes.py # frontend/src/components/KnowledgeGraph.tsx
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.
Closes #91
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit