Uh oh!
There was an error while loading. Please reload this page.
fix(learn): add a concept back after deleting it, and stop a stale backend answering - #536
fix(learn): add a concept back after deleting it, and stop a stale backend answering#536Darkest-Teddy wants to merge 7 commits into
Conversation
The backend was never the obstacle: add -> delete -> add of the same name
creates a fresh row every time. graph_nodes has no soft delete, delete_node
hard-deletes the row and its edges, and the 0023 UNIQUE has nothing left to
collide with. Verified against the real project.
The break is in the rail's add-concept composer:
cardCourseId = topicNode?.course_id || selectedCourseId || null
...
if (!label || !cardCourseId) return;
The course picker is "Course (optional)" and starts at "" ("No course"), so
unless the user arrived with ?course= or picked one, cardCourseId comes
ENTIRELY from the focused node. Deleting a concept clears the focus — so the
delete removes the one thing supplying the course id, and the add then
returns before doing anything: no request, no toast, no rollback, the
composer just sitting there with the name typed in.
addConcept now resolves through resolveAddConceptCourseId, which falls back
to the last course that did resolve, and says so when there has genuinely
never been one instead of no-op'ing. Exported, like resolveCardCourseId, so
the test exercises the resolution addConcept actually calls rather than a
mirror of it.
Also stops removeConcept swallowing its own failures. `.catch(() => {})` is
not best-effort, it's a lie: the node vanishes from the map while the row
survives, so re-adding the same name quietly MERGES into the undeleted row
and toasts "Merged into your existing X" for a concept the user watched
themselves delete. It now restores the node and says the delete failed.
Verified with tsc --noEmit. vitest cannot start on the local Node 20
(rolldown's styleText crash), so the new specs are CI-verified only.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>The toast said "Couldn't save the concept — it was removed" for a 401, a 500, a 422 and an unreachable backend alike. Four different problems, four different fixes, none of them guessable from the message — the catch discarded the ApiError it was handed. Chasing a real report of this cost several rounds of inference that the status code would have ended immediately: the route itself returns 200 and creates the row when called with the ids the client actually holds, so whatever the browser hit is above add_node, and the toast was the only witness. It now carries the status and the start of the body (the 500 path includes a request_id that ties it to a log), and both handlers log the full error to the console. Not instrumentation to be removed later: an error message that names a cause is what these two toasts should always have said. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The add-concept 404 was never a graph bug. A backend left running from the
MAIN working tree — older code, no create_node — was still answering :5000,
so POST /api/graph/{user_id}/nodes 404'd in the browser while returning 200
through TestClient in this worktree.
Windows lets a second process bind a port another process is already
listening on, and the FIRST binder keeps the traffic. Both restarts during
this session bound a second socket, logged "Application startup complete",
passed /api/health, and served nobody. Get-NetTCPConnection listed two
owning PIDs; the stale parent looked dead in Win32_Process while its uvicorn
reload child held the socket, so killing the parent alone did nothing.
`python main.py` now probes the port first and exits with the kill command
instead of starting a server that cannot receive a request. Dev entrypoint
only — containers and Railway import main:app and never reach it.
The tell, for next time: if a route you can read in routes/ is missing from
curl -s localhost:5000/openapi.json
you are talking to a different build, not looking at a routing bug.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reached
Next review available in:8 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (5)
📝 WalkthroughWalkthroughThe backend now checks development port occupancy before starting Uvicorn. The Learn screen retains course resolution after focus loss and improves concept save and deletion error handling. ChangesBackend startup
Learn graph operations
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | b5c7bc9 | Commit Preview URL Branch Preview URL | Aug 19 2026, 09:07 PM |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 1294-1295: Update the deletion error handling in Learn.tsx around
the ApiError check to include a truncated err.message alongside err.status in
the toast. Preserve the existing non-ApiError behavior and match the add failure
path’s response-detail formatting.
- Around line 1203-1206: Update Learn around resolveAddConceptCourseId and the
Add concept control so a single resolved add-course value is computed and reused
by both addConcept and the render condition, rather than gating the control
directly on cardCourseId. Preserve the fallback behavior when the picker has no
selection, and add a component test covering deletion of the focused concept
with no picker selection while confirming Add concept remains available.
🪄 Autofix
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: c58cf75a-76c6-43a2-9876-c9c6074f4f35
📒 Files selected for processing (3)
backend/main.pyfrontend/src/components/screens/Learn.graph.test.tsfrontend/src/components/screens/Learn.tsx
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
The tutor page had no quiz entry point at all — Learn.tsx did not mention
quiz anywhere. The only routes in were the nav's Quiz item, the dashboard,
the knowledge map and the notetaker, and every one of them drops the
session's context: the tutor knows exactly which concept the student is
working on, then hands them a page that asks them to pick it again.
Two buttons, one resolver:
focus card ("Quiz me") -> the rail's anchored concept
session toolbar ("Quiz me") -> the focused concept, else the session topic
`quizHref` prefers a node id, which screens/Quiz.tsx uses directly as
initialConceptId, and falls back to the topic NAME, which it resolves
against the concept list.
The placeholder guard is the part worth reading: an optimistically-added
concept carries a client-side `node-new-<ts>` id and a streamed one carries
`stream-<name>`, neither of which exists server-side. Passing either as
?concept= would preselect an id the quiz page cannot resolve — silently, as
an empty picker. Those fall through to the name, which resolves as soon as
the real row lands. Exported and tested like the other resolvers, so the
test covers what the buttons actually call.
Verified with tsc --noEmit. vitest cannot start on the local Node 20
(rolldown styleText crash), so the new specs are CI-verified only.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Jose-Gael-Cruz-Lopez
commented
Aug 19, 2026
Code review — learn concept re-add + port guardThe diagnosis here is excellent — the "stale process on :5000" write-up is the most useful thing in the PR, and FindingsP0[P0] {/* Add concept */}{cardCourseId&&(<divstyle={{padding: "4px 22px 24px"}}>Both Worse, the reported repro still fails. The new specs pass because they call the exported resolver directly and never render the gate. The gate is where the fix belongs: resolve the add-course once and gate the composer on that, showing which course the concept will land in. (CodeRabbit raised the gate; this adds the proof that the fallback branch is unreachable and that the original repro is unchanged.) P1[P1] A 404 on delete resurrects the node and toasts a false statement — deleteGraphNode(userId,nodeId).catch((err: unknown)=>{console.error("[removeConcept] failed",{ nodeId, err });if(!removedNode||/^(node-new-|stream-)/.test(nodeId))return;setGraphNodes(prev=>(prev.some(n=>n.id===nodeId) ? prev : [...prev,removedNode]));
...
constreason=errinstanceofApiError ? ` (${err.status})` : "";toast.error(`Couldn't delete “${removedNode.name}”${reason} — it's still on your map.`);The restore fires on any rejection, and 404 is precisely the status the endpoint returns for "the row is already gone": It compounds: delete X → 404 → user re-adds X (fresh row, new id) → the catch appends the old node object under its dead id and the map shows two "X" nodes, one of which doesn't exist. A 404 should be treated as success — nothing to restore. (The non-404 case is already handled correctly: the P2[P2] Hand-rolled error copy duplicates constreason=errinstanceofApiError
? `${err.status}${err.message ? ` — ${err.message.slice(0,160)}` : ""}`
: errinstanceofError&&err.message
? err.message.slice(0,160)
: "the request never completed";toast.error(`Couldn't save “${label}” (${reason}).`);
[P2] An undocumented quiz deep-link feature ships in a PR that says it excludes quiz work — exportfunctionquizHref(conceptId: string|null|undefined,topic: string|null|undefined,): string{
P3[P3] A failed delete restores the node but not the focus — setFocusedNodeId(cur=>(cur===nodeId ? null : cur));The catch at [P3] The port probe only covers IPv4 loopback — withsocket.socket(socket.AF_INET, socket.SOCK_STREAM) asprobe:
probe.settimeout(0.4)
ifprobe.connect_ex(("127.0.0.1", port)) !=0:
return
What's good
Verdict: request changes — the P0 leaves the reported bug reproducible on the default path, and the P1 introduces a new map/server divergence in the opposite direction. Review-only pass — no code changed and nothing fixed. Conventions checked against the Canopy |
…p a 404 resurrecting a node
The previous pass added `resolveAddConceptCourseId` so add-concept could
survive the delete that clears the focus — and then gated the composer that
calls it on `cardCourseId`, the exact value the fallback exists to work
around. Both `addConcept` call sites lived inside that gate, so the fallback
could only ever run while `cardCourseId` was already truthy: dead code, and
the reported bug ("delete a concept, add it back under the same name")
unchanged. Deleting the focused concept with the optional course picker at
its "" default unmounted the whole "+ Add concept" affordance, so the name
could not be typed back in at all.
The gate and `addConcept` now read ONE resolved value, `addCourseId`, so
they cannot disagree again. `lastCourseId` became state rather than a ref
because the gate reads it during render, and a ref mutated in an effect
produces no re-render. The composer names the course the concept will land
in, since `addCourseId` can be the remembered fallback rather than what the
focus card shows — otherwise the destination is a silent guess.
Also in removeConcept:
- A 404 is the endpoint's answer for "that row is already gone"
(routes/graph.py::remove_node, from graph_service.delete_node's
{"error": "Node not found"}), so restoring on it put a phantom concept
back on the map under a toast asserting it was "still on your map". After
the student re-added the same name it showed two, one of which existed
nowhere. `shouldRestoreFailedDelete` draws the line; the double-insert
guard stays for genuine failures.
- A restored node now gets its focus back. Restoring the node but not the
focus left the concept on the map with the rail's focus card — and so the
Remove button and the composer — belonging to a different concept.
- Both the add-failure and delete-failure toasts go through
lib/errorMessage's `humanizeError` instead of interpolating
`ApiError.message`, which is the RAW response body: the toast used to read
`Couldn't save "X" (404 - {"detail":"Not Found"})`, or 160 characters of a
proxy's HTML error page.
Learn.addConcept.test.tsx is a component test on purpose: the gap here was
in which value the render gate reads, which no resolver-level test could
see. Each of the four fixes was verified to fail the suite when reverted.The guard probed AF_INET/127.0.0.1 only. `localhost` resolves to both
127.0.0.1 and ::1 on any dual-stack box, and uvicorn binds whatever its
--host resolved to, so a stale server on ::1 (or on one specific interface)
walked straight past the check and the guard reported "port free" for exactly
the situation it was written to name — while `localhost:8000` in the browser
still reached the old server.
Both literals are probed now, with getaddrinfo("localhost") folded in on top
so a host whose localhost maps somewhere unusual is still covered, and an
unsupported family is skipped rather than raising. The SystemExit message
names the address that answered; without it the next person is told "already
serving" and has to guess which family to go hunting on.
Still dev-entrypoint only. tests/test_port_guard.py covers occupied IPv4,
occupied IPv6 (skipped where there is no IPv6 loopback), free, and the
message contents; the IPv6 case fails against the AF_INET-only probe.Jose-Gael-Cruz-Lopez
commented
Aug 19, 2026
Review fixes appliedEvery outstanding finding on this PR (human review + CodeRabbit) has been addressed and pushed. Blocker
Major
Minor / nits
NoteThe Verification — Fixes applied and verified locally against this branch head; each figure above is a command I ran, not an estimate. |
Split out of #534, which was carrying these three alongside unrelated quiz
work. Based on
main; no dependency on #533/#534.Reported as: "when deleting a concept node, i should be able to add it back
of the same name by add concept."
The reported bug was a stale server, not the graph
POST /api/graph/{user_id}/nodesreturned404 {"detail":"Not Found"}inthe browser while returning 200 through TestClient in the worktree. A backend
left running from a different working tree — older code, no
create_node—was still answering :5000.
Windows lets a second process bind a port another process is already
listening on, and the first binder keeps the traffic. Restarts bound a
second socket, logged "Application startup complete", passed
/api/health,and served nobody.
Get-NetTCPConnectionlisted two owning PIDs; the staleparent looked dead in
Win32_Processwhile its uvicorn reload child heldthe socket, so killing the parent alone did nothing.
python main.pynow probes the port first and exits with the kill command.Dev entrypoint only — containers and Railway import
main:appand neverreach it.
The tell, for next time: if a route you can read in
routes/is missing fromcurl -s localhost:5000/openapi.json, you are talking to a different build.Two real Learn bugs found on the way
Add-concept silently no-oped after a delete. The composer resolved its
course as
topicNode?.course_id || selectedCourseId || null. The picker is"Course (optional)" and starts at
"", so for anyone who hasn't chosen onethe course id came entirely from the focused node — and deleting a concept
clears the focus.
if (!label || !cardCourseId) returnthen bailed beforedoing anything: no request, no toast, no rollback.
resolveAddConceptCourseIdfalls back to the last course that did resolve, and says so when there has
genuinely never been one. Exported, like
resolveCardCourseId, so the testexercises the resolution
addConceptactually calls rather than a mirror.Failures were unreadable.
.catch(() => toast.error("Couldn't save the concept — it was removed"))discarded theApiError, giving a 401, a 404, a500 and an unreachable backend one identical sentence. Surfacing the status
and body is what produced the 404 +
request_idthat ended the hunt. Thedelete path had the same problem and worse:
.catch(() => {})let the nodevanish from the map while the row survived, so re-adding the same name
quietly merged into the undeleted row.
Verification
ruff checkcleanSystemExit, free → returns)tsc --noEmitclean on the identicalLearn.tsxstyleTextcrash), sothe new specs are CI-verified only
The backend was never at fault: add → delete → re-add of the same name
creates a fresh row every time — verified against the real project.
🤖 Generated with Claude Code
Summary by CodeRabbit