Uh oh!
There was an error while loading. Please reload this page.
feat: B10 stragglers — manual add-concept (#330) + rooms semantics (#405) - #485
Conversation
- graph_service.add_node: thin wrapper over apply_graph_update (dedup,
edges, analytics refresh stay in one place); resolves the anchor id to
its NAME (the edge machinery is name-keyed); reports merge-vs-create
via a pre-check; returns the canonical row.
- POST /api/graph/{user_id}/nodes (require_self; 422 on blank name/course;
course_id required — NULL-course dedup is always-distinct).
- Learn's rail composer now PERSISTS (was client-only, vanished on
refresh): optimistic node kept, canonical id swapped in via pure
reconcile helpers (lib/graphOptimistic, unit-tested), rollback + toast
on failure, merge toast on dedup.
- Tree gains the '+ Add concept' composer (single-course filter only),
anchored to the selected node when it belongs to the course; reloads
from DB truth; graph-add-concept* testids registered in doc + eslint
array (Tree.tsx entered the enforcement list with zero baselined debt).
- Journey e2e/tree-add-concept.spec.ts: persist → render (a11y list by
node id) → case-drifted re-add merges, original row survives.
Part of #330.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>- Migration 0038: backfill owner_id := created_by (+ NOT NULL) — real,
transferable ownership; created_by stays the immutable creator record.
is_public DEFAULT false NOT NULL (null was neither); updated_at DEFAULT
now() + backfill from created_at.
- create_room populates owner_id/topic/course/is_public (CreateRoomBody
widened); kick authorization keys on owner_id; membership changes touch
updated_at (_touch_room — message traffic left to the 0033 realtime flow).
- Public surface: GET /api/social/public-rooms (explicit projection — the
payload cannot carry invite_code) + invite-less
POST /api/social/public-rooms/{id}/join (403 for private, idempotent).
- Frontend: create form gains topic/course/Public fields; sidebar renders
topic + Public badge; a Public-rooms discovery list with one-click Join.
Testids registered in the doc; Tree.tsx's pre-existing untagged elements
baselined per the documented recipe.
- Journey room-public.spec.ts: UI create with topic+public → DB semantics
assert → second user context joins invite-less → membership row.
Part of #405.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>This pull request has been ignored for the connected project Preview Branches by Supabase. |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | c712274 | Commit Preview URL Branch Preview URL | Jul 30 2026, 07:23 PM |
Warning Review limit reached
Next review available in:39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR adds persisted manual graph-concept creation with optimistic reconciliation, and extends social rooms with ownership, metadata, public discovery, invite-less joining, and membership timestamp updates. It also adds backend, frontend, API, and end-to-end coverage. ChangesManual graph concepts
Public rooms
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Tree
participant addGraphNode
participant create_node
participant add_node
participant graph_nodes
Tree->>addGraphNode: submit concept and course
addGraphNode->>create_node: POST node request
create_node->>add_node: validated node data
add_node->>graph_nodes: create or merge concept
graph_nodes-->>add_node: canonical node
add_node-->>create_node: node and merge flag
create_node-->>addGraphNode: response
addGraphNode-->>Tree: reconcile optimistic graph state
sequenceDiagram
participant Social
participant listPublicRooms
participant list_public_rooms
participant joinPublicRoom
participant join_public_room
participant room_members
Social->>listPublicRooms: request discoverable rooms
listPublicRooms->>list_public_rooms: GET public rooms
list_public_rooms-->>Social: metadata without invite codes
Social->>joinPublicRoom: select room
joinPublicRoom->>join_public_room: POST user_id
join_public_room->>room_members: insert membership if absent
join_public_room-->>Social: joined room response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
frontend/src/components/screens/Social.tsx (1)
36-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLocal
Roomtype overlaps withPublicRoomfromtypes.ts.
Roomhere duplicates several fields (topic,course,is_public,created_by) that also exist on thePublicRoominterface infrontend/src/lib/types.ts. Not blocking, but worth considering a shared base type to avoid the two drifting apart as more room fields are added.🤖 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/Social.tsx` around lines 36 - 41, Replace the local Room type in Social.tsx with the shared PublicRoom type from frontend/src/lib/types.ts, or derive Room from it if the component needs additional fields. Remove duplicated room-field declarations and update affected usages to rely on the shared definition.backend/db/migrations/0038_rooms_semantics.sql (1)
19-19: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
SET NOT NULLtakes anACCESS EXCLUSIVElock for the full validation scan.Both
ALTER COLUMN owner_id SET NOT NULLandALTER COLUMN is_public SET NOT NULLblock all reads/writes onroomsfor the duration of the table scan used to validate the constraint. The static-analysis hint (squawk) flags this; the safer pattern isADD CONSTRAINT ... CHECK (col IS NOT NULL) NOT VALIDfollowed by a separateVALIDATE CONSTRAINT, which only takes a brief lock and allows concurrent reads/writes during validation. Givenroomsis presumably small today, impact is likely minor, but this is the correct pattern going forward as the table grows.♻️ Safer non-blocking pattern
-ALTER TABLE rooms ALTER COLUMN owner_id SET NOT NULL;+ALTER TABLE rooms ADD CONSTRAINT rooms_owner_id_not_null CHECK (owner_id IS NOT NULL) NOT VALID;+ALTER TABLE rooms VALIDATE CONSTRAINT rooms_owner_id_not_null;Same pattern applies to
is_public.Also applies to: 23-23
🤖 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/db/migrations/0038_rooms_semantics.sql` at line 19, Replace the direct SET NOT NULL operations for rooms.owner_id and rooms.is_public with named CHECK constraints using NOT VALID, then validate each constraint in separate statements. Preserve the enforced non-null behavior while using the lower-lock validation pattern for both columns.Source: Linters/SAST tools
backend/routes/social.py (1)
65-88: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winN+1 query for member counts in public room listing.
For every public room, this issues a separate
room_membersselect (line 71-73) to computemember_count. This scales linearly with the number of public rooms per request; as public-room discovery grows, this becomes a hot-path performance concern.♻️ Batch the membership counts
- rooms = table("rooms").select(- "id,name,topic,course,owner_id,created_by,created_at,updated_at,is_public",- filters={"is_public": "eq.true"},- ) or []- out = []- for room in rooms:- members = table("room_members").select(- "user_id", filters={"room_id": f"eq.{room['id']}"},- ) or []+ rooms = table("rooms").select(+ "id,name,topic,course,owner_id,created_by,created_at,updated_at,is_public",+ filters={"is_public": "eq.true"},+ ) or []+ room_ids = [r["id"] for r in rooms]+ all_members = table("room_members").select(+ "room_id", filters={"room_id": f"in.({','.join(room_ids)})"},+ ) if room_ids else []+ counts: dict = {}+ for m in all_members:+ counts[m["room_id"]] = counts.get(m["room_id"], 0) + 1+ out = []+ for room in rooms:+ members_count = counts.get(room["id"], 0)Adjust to whatever batching/
in.filter syntaxtable()supports.🤖 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/routes/social.py` around lines 65 - 88, Replace the per-room room_members query in the public room listing with a batched membership lookup using a single in-style filter for all public room IDs, then aggregate counts by room_id and use those counts when building each payload in the existing rooms loop. Preserve zero counts for rooms with no members and the current explicit response projection.
🤖 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 `@backend/services/graph_service.py`:
- Around line 604-625: Make normalized concept deduplication atomic by adding an
append-only database migration with a uniqueness constraint/index on the
normalized concept key, then update the graph-node creation flow around
apply_graph_update to upsert against that constraint rather than relying on the
pre-check. Derive already_existed from the atomic upsert result, preserving the
existing edge creation behavior for newly inserted nodes.
In `@backend/tests/test_graph_add_node.py`:
- Around line 30-42: Replace the local _graph_nodes_mock MagicMock factory with
the shared mocked-Supabase fixture from tests/conftest.py, configuring its
graph_nodes.select behavior with the required select_side_effect and preserving
empty results for other tables through the fixture’s existing setup.
In `@frontend/src/components/screens/Social.tsx`:
- Around line 1239-1241: Update the member-count label in the sidebar room
rendering to use the same singular/plural logic as PublicRoomsList, displaying
“member” for a count of one and “members” otherwise while preserving the
existing topic prefix.
In `@frontend/src/lib/graphOptimistic.ts`:
- Around line 45-50: Update the canonical-node reconciliation branch in the
graph optimistic helper to copy the backend-normalized concept name into the
optimistic node, alongside the existing canonical id and mastery fields. Add a
test case in the graph optimistic tests covering an optimistic name with extra
spacing and asserting the reconciled node uses the canonical normalized name.
---
Nitpick comments:
In `@backend/db/migrations/0038_rooms_semantics.sql`:
- Line 19: Replace the direct SET NOT NULL operations for rooms.owner_id and
rooms.is_public with named CHECK constraints using NOT VALID, then validate each
constraint in separate statements. Preserve the enforced non-null behavior while
using the lower-lock validation pattern for both columns.
In `@backend/routes/social.py`:
- Around line 65-88: Replace the per-room room_members query in the public room
listing with a batched membership lookup using a single in-style filter for all
public room IDs, then aggregate counts by room_id and use those counts when
building each payload in the existing rooms loop. Preserve zero counts for rooms
with no members and the current explicit response projection.
In `@frontend/src/components/screens/Social.tsx`:
- Around line 36-41: Replace the local Room type in Social.tsx with the shared
PublicRoom type from frontend/src/lib/types.ts, or derive Room from it if the
component needs additional fields. Remove duplicated room-field declarations and
update affected usages to rely on the shared definition.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e864ff8a-fd63-4a93-9af3-265960cf6910
📒 Files selected for processing (21)
backend/db/migrations/0038_rooms_semantics.sqlbackend/models/__init__.pybackend/routes/graph.pybackend/routes/social.pybackend/services/graph_service.pybackend/tests/test_graph_add_node.pybackend/tests/test_social_rooms.pydocs/frontend-testids.mdfrontend/e2e/room-public.spec.tsfrontend/e2e/tree-add-concept.spec.tsfrontend/eslint-suppressions.jsonfrontend/eslint.config.mjsfrontend/src/components/screens/Learn.tsxfrontend/src/components/screens/Social.tsxfrontend/src/components/screens/Tree.tsxfrontend/src/lib/api.tsfrontend/src/lib/graphApi.test.tsfrontend/src/lib/graphOptimistic.test.tsfrontend/src/lib/graphOptimistic.tsfrontend/src/lib/socialApi.test.tsfrontend/src/lib/types.ts
| pre = table("graph_nodes").select("id,concept_name", filters=scope) or [] | ||
| already_existed = any( | ||
| _normalize_concept(r.get("concept_name") or "") == norm for r in pre | ||
| ) | ||
| update: dict = { | ||
| "new_nodes": [ | ||
| {"concept_name": name, "course_id": course_id, "initial_mastery": initial_mastery}, | ||
| ], | ||
| } | ||
| if anchor_node_id: | ||
| anchor_rows = table("graph_nodes").select( | ||
| "id,concept_name", | ||
| filters={"id": f"eq.{anchor_node_id}", "user_id": f"eq.{user_id}"}, | ||
| ) or [] | ||
| anchor_name = (anchor_rows[0].get("concept_name") if anchor_rows else "") or "" | ||
| if anchor_name and _normalize_concept(anchor_name) != norm: | ||
| update["new_edges"] = [ | ||
| {"source": anchor_name, "target": name, "relationship_type": "related", "strength": 0.5}, | ||
| ] | ||
| apply_graph_update(user_id, update, course_id=course_id) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make normalized concept deduplication atomic.
The pre-check races with the raw-name upsert: concurrent Recursion and recursion requests both miss, do not conflict on concept_name, and create duplicate normalized nodes. Enforce the normalized key with a database uniqueness constraint in an append-only migration, upsert against it, and derive already_existed from that atomic result.
🤖 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/services/graph_service.py` around lines 604 - 625, Make normalized
concept deduplication atomic by adding an append-only database migration with a
uniqueness constraint/index on the normalized concept key, then update the
graph-node creation flow around apply_graph_update to upsert against that
constraint rather than relying on the pre-check. Derive already_existed from the
atomic upsert result, preserving the existing edge creation behavior for newly
inserted nodes.
Source: Coding guidelines
| def _graph_nodes_mock(select_side_effect): | ||
| """One cached graph_nodes mock whose .select returns follow the given | ||
| sequence; other tables return empty lists.""" | ||
| nodes = MagicMock() | ||
| nodes.select.side_effect = select_side_effect | ||
| def factory(name): | ||
| if name == "graph_nodes": | ||
| return nodes | ||
| other = MagicMock() | ||
| other.select.return_value = [] | ||
| return other | ||
| return factory, nodes |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the shared Supabase test fixture.
This local MagicMock factory bypasses the repository’s shared mocked-Supabase setup, allowing service-test contracts to drift. Configure the shared fixture instead.
As per coding guidelines, “Backend tests belong under backend/tests/ and run with pytest; use the shared fixtures in tests/conftest.py for mocked Supabase and Gemini services.”
🤖 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_graph_add_node.py` around lines 30 - 42, Replace the local
_graph_nodes_mock MagicMock factory with the shared mocked-Supabase fixture from
tests/conftest.py, configuring its graph_nodes.select behavior with the required
select_side_effect and preserving empty results for other tables through the
fixture’s existing setup.
Source: Coding guidelines
| <div style={{ fontSize: 11, color: "var(--text-muted)" }}> | ||
| {r.topic ? `${r.topic} · ` : ""}{r.member_count} members | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Sidebar always shows "members" even for a single member.
Right after room creation, a room has exactly one member (the creator), so this renders "1 members." The sibling PublicRoomsList component (line 246) already handles the singular/plural correctly — reuse that logic here.
🐛 Proposed fix
<div style={{ fontSize: 11, color: "var(--text-muted)" }}>
- {r.topic ? `${r.topic} · ` : ""}{r.member_count} members+ {r.topic ? `${r.topic} · ` : ""}{r.member_count} member{r.member_count === 1 ? "" : "s"}
</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <divstyle={{fontSize: 11,color: "var(--text-muted)"}}> | |
| {r.topic ? `${r.topic} · ` : ""}{r.member_count}members | |
| </div> | |
| <divstyle={{fontSize: 11,color: "var(--text-muted)"}}> | |
| {r.topic ? `${r.topic} · ` : ""}{r.member_count}member{r.member_count===1 ? "" : "s"} | |
| </div> |
🤖 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/Social.tsx` around lines 1239 - 1241, Update
the member-count label in the sidebar room rendering to use the same
singular/plural logic as PublicRoomsList, displaying “member” for a count of one
and “members” otherwise while preserving the existing topic prefix.
Uh oh!
There was an error while loading. Please reload this page.
AndresL230
commented
Jul 30, 2026
Code reviewFound 1 issue:
Sapling/backend/db/migrations/0038_rooms_semantics.sql Lines 21 to 26 in 60cbdcc Sapling/backend/db/seed_local_rich.py Lines 448 to 456 in 60cbdcc Fixed, along with eight sub-bar findings the same pass raised (all fixed regardless of score, per the standing rule):
One finding was documented rather than fixed: 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
BLOCKER: 0038 makes rooms.owner_id NOT NULL with no default, but seed_local_rich never set it — a from-empty replay (make e2e-up, CI's e2e.yml) failed the first rooms INSERT and aborted the stack boot. Masked on existing volumes, where the upsert becomes an UPDATE. Also: - both join paths upsert on the room_members PK instead of check-then- insert (a double-click raced to a raw 500); Join gains an in-flight guard - reconcileNodes absorbs live stream-<name> placeholders by normalized name so a manual re-add can't render two nodes for one merged row; edges retargeted too; rollback now uses dropOptimisticConcept - add_node's anchor lookup is course-scoped (a foreign anchor resolved by name INSIDE the target course could wire to the wrong node) - Social's leader/kick UI keys on owner_id (backend already did); RoomOverviewData declares it - PublicRoomsList toasts its fetch failure instead of rendering an empty list, and revalidates itself after a join - optimistic temp ids carry a monotonic suffix (Date.now() is ms-resolution and this path has no in-flight guard) - AddNodeBody docstring no longer inverts UNIQUE NULLS NOT DISTINCT; the dedup caveat (Python-side case folding vs a case-sensitive constraint) is documented rather than overclaimed; testids doc lists Tree's real baselined elements - room-public.spec.ts uses the seeded name for rich-user-second Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The course pill's label is a prefix of the graph node's a11y activate
button ('MATH210 - Linear Algebra'), so the loose regex matched two
elements once the graph rendered — strict-mode violation, caught by the
cycle's first run of this journey.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Part of #330 and part of #405 (both left open per closure policy — Andres verifies the UI surfaces and closes). Completes B10 alongside the #323 audit (closed with follow-ups #481–#484).
#330 — manually add a concept
graph_service.add_node: thin wrapper overapply_graph_update(dedup/edges/analytics-refresh stay in one place); resolves the anchor id to its NAME (the edge machinery is name-keyed); reports merge-vs-create; returns the canonical row.POST /api/graph/{user_id}/nodes(require_self, 422 on blank name;course_idrequired — NULL-course dedup is always-distinct).tree-add-concept.spec.ts: persist → render (a11y list by node id) → case-drifted re-add merges, original row survives.#405 — rooms semantics (Andres's decision: real ownership + public rooms)
owner_id := created_bybackfill + NOT NULL (ownership is real and transferable later;created_bystays the immutable creator record),is_publicDEFAULT false NOT NULL,updated_atDEFAULT now() + backfill.create_roompopulates owner/topic/course/is_public; kick authorization keys on owner_id; membership changes touchupdated_at.GET /api/social/public-rooms(explicit projection — cannot leakinvite_code) + invite-lessPOST /api/social/public-rooms/{id}/join(403 private, idempotent). Frontend: create form gains topic/course/Public, sidebar shows topic + Public badge, and a Public-rooms discovery list with one-click Join. A fuller discovery surface stays future work per the decision.room-public.spec.ts: UI create with topic+public → DB semantics assert → second user joins invite-less → membership row.Tests
Backend: 17 new (add_node service/route ×8, rooms semantics ×9); suite 1525 passed; ruff clean. Frontend: 397 passed / tsc / lint 0 errors (rebased over #480's suppressions prune — converged). Two promoted journeys. Full local e2e cycle before merge.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests