Port entity graph to server-side BFS endpoint#88
Open
damienriehl wants to merge 44 commits intodevfrom
Open
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMigrates client-side graph construction to a backend-driven EntityGraph API, introduces a client-only ELK layout hook, replaces inline ELK/layout code with the hook, adds a full-screen EntityGraph modal, updates React Flow node/edge rendering and interaction handlers, and removes legacy build/elk modules and their tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Editor as StandardEditorLayout
participant Modal as EntityGraphModal
participant Graph as OntologyGraph
participant Hook as useGraphData
participant API as graphApi
participant Backend as BackendAPI
participant Layout as useELKLayout
Editor->>Modal: open modal (focusIri)
Modal->>Graph: mount with focusIri, accessToken
Graph->>Hook: init fetch
Hook->>API: getEntityGraph(projectId, focusIri, {ancestorsDepth:5, descendantsDepth:?})
API->>Backend: GET /api/v1/projects/{id}/ontology/classes/graph
Backend-->>API: EntityGraphResponse (nodes, edges, truncated)
API-->>Hook: EntityGraphResponse
Hook->>Layout: runLayout(EntityGraphResponse, direction)
Layout->>Layout: dynamic import elkjs & compute positions
Layout-->>Hook: layoutNodes/layoutEdges, isLayouting false
Hook-->>Graph: update graphData + layoutNodes/layoutEdges
Graph-->>User: render positioned graph
sequenceDiagram
participant GraphUser as User
participant GraphComp as OntologyGraph
participant Hook as useGraphData
participant API as graphApi
GraphUser->>GraphComp: click unexplored node
GraphComp->>Hook: expandNode(iri)
Hook->>API: getEntityGraph(projectId, iri, {ancestorsDepth:1, descendantsDepth:1, maxNodes:50})
API->>Backend: GET /api/v1/projects/{id}/ontology/classes/graph
Backend-->>API: expansion EntityGraphResponse
API-->>Hook: expansion response
Hook->>Hook: mergeExpansions(existing, expansion) (dedupe ids)
Hook-->>GraphComp: updated graphData (merged)
GraphComp->>Layout: runLayout(mergedGraphData)
Layout-->>GraphComp: new positions
GraphComp-->>GraphUser: re-render with expanded nodes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. Comment |
This was referenced Apr 6, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
components/editor/standard/StandardEditorLayout.tsx (1)
456-460: Use the tree label for the modal title.
getLocalName(selectedIri)can reduce the fullscreen header to a raw fragment/slug even when the class tree already has a better label for the same entity. SincetreeLabelHintsRecordis already in scope here, use that first and keepgetLocalName()as the fallback.📝 Suggested tweak
<EntityGraphModal focusIri={selectedIri} - label={getLocalName(selectedIri)} + label={treeLabelHintsRecord[selectedIri] ?? getLocalName(selectedIri)} projectId={projectId} branch={activeBranch}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/editor/standard/StandardEditorLayout.tsx` around lines 456 - 460, The modal title currently uses getLocalName(selectedIri) which can be a raw fragment; change the label prop passed to EntityGraphModal to prefer treeLabelHintsRecord[selectedIri] (or an appropriate lookup like treeLabelHintsRecord.get(selectedIri)) and fall back to getLocalName(selectedIri) when no tree label exists—update the JSX where showGraphModal && selectedIri render EntityGraphModal (props focusIri and label) to use this priority lookup.__tests__/lib/graph/useELKLayout.test.ts (1)
118-129: This test doesn't verify the width logic yet.It currently passes as long as one node is returned, so a regression in the dynamic-width formula would not fail the suite. Capture the layout input and assert that the long label produces a width larger than the default node width.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/graph/useELKLayout.test.ts` around lines 118 - 129, The test currently only checks that a node exists but not that its width scales with label length; update the test to assert the computed width for the long label is larger than a baseline width by (1) creating a baseline layout run using makeGraphResponse(1) with a short label (or the original label) and calling result.current.runLayout to get baselineNodeWidth, (2) then set data.nodes[0].label = "A very long label for testing width", run result.current.runLayout again, and (3) assert that result.current.nodes[0].width (or the property used by useELKLayout) is greater than the baselineNodeWidth; reference useELKLayout, runLayout, makeGraphResponse, and result.current.nodes to locate and modify the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/graph/EntityGraphModal.tsx`:
- Around line 43-45: The modal in EntityGraphModal.tsx is rendered with plain
divs so screen readers and keyboard users aren't informed or trapped; update the
root modal container (the top-level div in the component) to include
role="dialog" and aria-modal="true", add an id on the title element and
reference it via aria-labelledby on the dialog, and implement focus management
in the component lifecycle (on mount move focus into the modal—e.g., to the
dialog container or the title/first focusable element—and on unmount restore
focus to the previously focused element); apply the same attributes and focus
behavior for the other modal block referenced around lines 62-68.
In `@components/graph/OntologyGraph.tsx`:
- Around line 183-195: The current handleNodeClick uses expandedNodes (seeded
with focusIri) to decide navigation, which causes single clicks to navigate;
remove that navigation path so single clicks only expand: in handleNodeClick
(and analogous code around lines ~198-203) do not call onNavigateToClass or
check expandedNodes.has(node.id) for navigation—only call expandNode(node.id)
and add to expandedNodes; move any navigation logic to the double-click handler
(onNodeDoubleClick) so onNavigateToClass is invoked only there; ensure focusIri
is not used to trigger navigation on first click by keeping it out of the
single-click navigation branch.
In `@lib/api/graph.ts`:
- Around line 47-63: The getEntityGraph helper doesn't forward the session
access token, so add an optional accessToken parameter to getEntityGraph (e.g.,
getEntityGraph: (projectId: string, classIri: string, options: FetchGraphOptions
= {}, accessToken?: string) => ...) and, when accessToken is provided, include
an Authorization header ("Bearer <token>") in the api.get call by adding
headers: { Authorization: `Bearer ${accessToken}` } to the request config
alongside params; keep existing params unchanged so callers can pass
session.accessToken through to authenticated graph endpoints.
In `@lib/graph/useELKLayout.ts`:
- Around line 27-100: runLayout can finish out-of-order and overwrite newer
layouts; add a simple run-id guard using a ref (e.g. layoutRunRef) that you
increment before starting a layout in runLayout and capture as localRunId; after
awaiting elk.layout and before calling setNodes/setEdges check that
layoutRunRef.current === localRunId and bail out if not. Update runLayout to
read/increment layoutRunRef (useRef<number>()), capture localRunId at start, and
skip applying the results when stale so only the latest completion updates the
state.
In `@lib/hooks/useGraphData.ts`:
- Around line 106-109: resetGraph currently only clears expandedNodes and
setGraphData but doesn't cause the initial fetch to rerun, leaving the hook
empty; update resetGraph to trigger the baseline BFS fetch by either calling the
existing fetch function directly or by bumping a small state key used in the
fetch effect dependencies. Concretely: add a resetKey state (e.g. const
[resetKey, setResetKey] = useState(0)), include resetKey in the useEffect that
performs the initial BFS/fetch, and then change resetGraph (the useCallback) to
setExpandedNodes to new Set(), setGraphData(null), and setResetKey(k => k + 1)
so the baseline request runs again; alternatively, call the named
fetchBaselineGraph function from resetGraph if that function exists in the
module.
- Around line 67-101: Move the immediate promotion to expanded out of the
pre-fetch path: create a separate in-flight set (e.g., expandingNodes.current)
and add iri to expandingNodes before calling graphApi.getEntityGraph; call
setGraphData inside the .then and, only after the updater successfully merges
the returned newData into the current graph (and confirming the updater's prev
still corresponds to the same graph instance/focus), remove iri from
expandingNodes and add it to expandedNodes.current; ensure failures (.catch)
only clear expandingNodes and never add to expandedNodes so transient errors
remain retryable and in-flight responses can't mark nodes expanded for a
different/reset graph (referencing expandedNodes.current,
expandingNodes.current, graphApi.getEntityGraph, setGraphData, and resetGraph).
- Around line 84-96: When merging expansion results in useGraphData.ts (the
return block that builds the new state from prev and newData), ensure the
truncation metadata is preserved by OR-ing the flags instead of keeping only
prev.truncated; update the returned object to set truncated: prev.truncated ||
newData.truncated (so any expansion that hit maxNodes surfaces as truncated),
leaving the rest of the merge (nodes, edges, total_concept_count) unchanged.
- Around line 27-63: Replace the manual useEffect/useState fetch with a single
useQuery from `@tanstack/react-query`: call useQuery with a queryKey like
['entityGraph', projectId, focusIri, branch, showDescendants], a queryFn that
invokes graphApi.getEntityGraph(projectId, focusIri, { branch, ancestorsDepth:
5, descendantsDepth: showDescendants ? 2 : 0 }), enabled: !!focusIri, and an
appropriate staleTime; remove the cancelled logic and local isLoading/graphData
state and instead use the query's data/isLoading/error; if you need to reset
expandedNodes when focusIri changes, do that in the useQuery onSuccess or via an
effect that watches focusIri and sets expandedNodes.current = new
Set([focusIri]); also add the necessary import for useQuery.
---
Nitpick comments:
In `@__tests__/lib/graph/useELKLayout.test.ts`:
- Around line 118-129: The test currently only checks that a node exists but not
that its width scales with label length; update the test to assert the computed
width for the long label is larger than a baseline width by (1) creating a
baseline layout run using makeGraphResponse(1) with a short label (or the
original label) and calling result.current.runLayout to get baselineNodeWidth,
(2) then set data.nodes[0].label = "A very long label for testing width", run
result.current.runLayout again, and (3) assert that
result.current.nodes[0].width (or the property used by useELKLayout) is greater
than the baselineNodeWidth; reference useELKLayout, runLayout,
makeGraphResponse, and result.current.nodes to locate and modify the test.
In `@components/editor/standard/StandardEditorLayout.tsx`:
- Around line 456-460: The modal title currently uses getLocalName(selectedIri)
which can be a raw fragment; change the label prop passed to EntityGraphModal to
prefer treeLabelHintsRecord[selectedIri] (or an appropriate lookup like
treeLabelHintsRecord.get(selectedIri)) and fall back to
getLocalName(selectedIri) when no tree label exists—update the JSX where
showGraphModal && selectedIri render EntityGraphModal (props focusIri and label)
to use this priority lookup.
🪄 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
Run ID: cb0cd069-2340-4302-9294-7da0e8a3f775
📒 Files selected for processing (17)
__tests__/lib/graph/buildGraphData.test.ts__tests__/lib/graph/useELKLayout.test.ts__tests__/lib/graph/utils.test.ts__tests__/lib/hooks/useGraphData.test.tscomponents/editor/developer/DeveloperEditorLayout.tsxcomponents/editor/standard/StandardEditorLayout.tsxcomponents/graph/EntityGraphModal.tsxcomponents/graph/OntologyEdge.tsxcomponents/graph/OntologyGraph.tsxcomponents/graph/OntologyNode.tsxlib/api/graph.tslib/graph/buildGraphData.tslib/graph/elkLayout.tslib/graph/useELKLayout.tslib/graph/utils.tslib/hooks/useGraphData.tstsconfig.json
💤 Files with no reviewable changes (3)
- lib/graph/elkLayout.ts
- tests/lib/graph/buildGraphData.test.ts
- lib/graph/buildGraphData.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/graph/useELKLayout.ts (1)
27-110:⚠️ Potential issue | 🟠 MajorPrevent stale async layouts from overwriting newer results.
runLayoutcan resolve out-of-order during rapid interactions; older runs still callsetNodes/setEdges(Line 106, Line 107) and can replace newer layout state. Add a run-id guard so only the latest completion mutates state.🔧 Suggested fix
import { useCallback, useState } from "react"; +import { useRef } from "react"; @@ export function useELKLayout(): LayoutResult { const [nodes, setNodes] = useState<Node<OntologyNodeData>[]>([]); const [edges, setEdges] = useState<Edge<OntologyEdgeData>[]>([]); const [isLayouting, setIsLayouting] = useState(false); + const layoutRunId = useRef(0); @@ const runLayout = useCallback( async (data: EntityGraphResponse, direction: LayoutDirection = "TB") => { + const runId = ++layoutRunId.current; setIsLayouting(true); @@ - setNodes(layoutedNodes); - setEdges(layoutedEdges); + if (runId !== layoutRunId.current) return; + setNodes(layoutedNodes); + setEdges(layoutedEdges); } finally { - setIsLayouting(false); + if (runId === layoutRunId.current) { + setIsLayouting(false); + } } }, [], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/graph/useELKLayout.ts` around lines 27 - 110, runLayout can finish out-of-order and overwrite newer layouts; add a run-id guard using a ref (e.g., latestLayoutRunRef) that you increment at the start of runLayout and capture into a local currentRunId, then only call setNodes and setEdges if currentRunId === latestLayoutRunRef.current, and in the finally block only call setIsLayouting(false) when the same guard matches so stale async results do not mutate state; reference runLayout, setNodes, setEdges, setIsLayouting and the new latestLayoutRunRef to implement this check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/graph/OntologyNode.tsx`:
- Line 95: The handles in OntologyNode are hardcoded to Position.Top/Bottom
causing bad routing in LR mode; update the ELK layout hook to include the
current layout direction (e.g., "TB" or "LR") on each node's data (add a
layoutDirection field to OntologyNodeData), then in the OntologyNode component
use that data to choose handle positions (use Position.Left/Right when
layoutDirection === "LR", otherwise Top/Bottom) so edges anchor correctly;
change any references in the ELK node creation to set node.data.layoutDirection
= direction.
---
Duplicate comments:
In `@lib/graph/useELKLayout.ts`:
- Around line 27-110: runLayout can finish out-of-order and overwrite newer
layouts; add a run-id guard using a ref (e.g., latestLayoutRunRef) that you
increment at the start of runLayout and capture into a local currentRunId, then
only call setNodes and setEdges if currentRunId === latestLayoutRunRef.current,
and in the finally block only call setIsLayouting(false) when the same guard
matches so stale async results do not mutate state; reference runLayout,
setNodes, setEdges, setIsLayouting and the new latestLayoutRunRef to implement
this check.
🪄 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
Run ID: f124f848-11c5-451d-ac3d-780138d5063e
📒 Files selected for processing (5)
components/editor/standard/StandardEditorLayout.tsxcomponents/graph/OntologyGraph.tsxcomponents/graph/OntologyNode.tsxlib/graph/types.tslib/graph/useELKLayout.ts
✅ Files skipped from review due to trivial changes (1)
- lib/graph/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- components/graph/OntologyGraph.tsx
Contributor
Author
Updates since initial PRBug fixes (from production testing)
seeAlso multi-root support
Root alignment & styling
Production verificationVerified on https://ontokit.openlegalstandard.org with "Motion to Dismiss":
Companion backend PRMerged on FOLIO (alea-institute) |
damienriehl
force-pushed
the
entity-graph-pr
branch
from
6f380d4 to
78beeae
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
lib/hooks/useGraphData.ts (4)
84-96:⚠️ Potential issue | 🟠 MajorPreserve
truncatedwhen expansion results are merged.This merged object keeps
prev.truncated, so an expansion that hitsmaxNodesstill surfaces asfalseand hides the incomplete-graph warning.Minimal fix
return { ...prev, + truncated: prev.truncated || newData.truncated, nodes: [ ...prev.nodes, ...newData.nodes.filter((n) => !existingNodeIds.has(n.id)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useGraphData.ts` around lines 84 - 96, The merged graph object is dropping the truncated flag, so keep it when merging results in useGraphData (the block that returns {...prev, nodes: ..., edges: ..., total_concept_count: ...}). Add a truncated property to the returned object using a boolean union of previous and new results (e.g., truncated: prev.truncated || newData.truncated) so any expansion that hit maxNodes remains marked truncated; reference the variables prev and newData and the keys nodes, edges, total_concept_count, and truncated when making the change.
106-109:⚠️ Potential issue | 🟠 Major
resetGraph()should restore the baseline graph instead of blanking it.Clearing local state here does not re-run the initial BFS request because none of the fetch dependencies change. The reset action leaves the graph empty until some other prop changes; trigger the baseline fetch here, or refetch the baseline query once this hook is on React Query.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useGraphData.ts` around lines 106 - 109, The resetGraph function currently clears expandedNodes and sets graphData to null, leaving the UI empty; instead restore the baseline graph by invoking the baseline fetch/refetch logic from this hook: inside resetGraph (the useCallback) keep expandedNodes.current = new Set(), then call the existing baseline loader/refetch function (e.g., the BFS initial request helper or the react-query baseline query's refetch/mutate method used elsewhere in this hook) rather than only setGraphData(null), ensuring the same code path that populates graphData on mount runs again; reference the resetGraph callback, setGraphData, expandedNodes.current, and the baseline BFS/react-query refetch function so the reset action re-populates the initial graph.
27-63: 🛠️ Refactor suggestion | 🟠 MajorMove this hook onto React Query.
This is still hand-rolling server state with
useEffectand local state, so it misses the repo's cache/dedup/retry conventions for API-backed graph data. Converting the baseline fetch touseQueryand treating expansion as a mutation or query update will also simplify the reset/refetch flow.As per coding guidelines, "Use React Query (
@tanstack/react-query) for server state management."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useGraphData.ts` around lines 27 - 63, The hook is hand-rolling server state with useEffect and local state; replace the fetch logic in useGraphData to use React Query's useQuery for graphApi.getEntityGraph (use a query key that includes projectId, focusIri, branch and showDescendants), remove manual setIsLoading and graphData state and consume isLoading/data from useQuery, drop the cancelled cleanup, initialize expandedNodes.current when focusIri changes (or store it in React state), and handle expansions as a mutation or by calling queryClient.setQueryData / invalidateQueries to update the cached query result; keep references to graphApi.getEntityGraph, useGraphData, expandedNodes, setShowDescendants and branch when migrating.
66-101:⚠️ Potential issue | 🟠 MajorOnly promote a node to expanded after its response is merged.
expandedNodes.current.add(iri)happens before the request resolves. That makes transient failures non-retryable, and a late response can still merge into a different focus graph, afterresetGraph(), or when the focus is cleared because the result is never scoped to the graph instance that started it. Keep a separate in-flight set and only move a node intoexpandedNodesonce the updater accepts the response for the current graph.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useGraphData.ts` around lines 66 - 101, The code prematurely calls expandedNodes.current.add(iri) before the async getEntityGraph response is merged, making failures non-retryable and allowing late responses to corrupt a different graph; fix by introducing an in-flight tracker (e.g., inFlightExpanded set) and only add iri to expandedNodes inside the setGraphData updater after verifying the updater is merging the response for the current graph instance (i.e., prev is non-null and matches expected focus/state), remove iri from inFlightExpanded on failure or after merge, and scope the merge by checking within the setGraphData callback that the response still applies before updating nodes/edges; update usages around getEntityGraph, setGraphData, expandedNodes.current.add, and resetGraph accordingly.components/graph/EntityGraphModal.tsx (1)
43-68:⚠️ Potential issue | 🟠 MajorAdd real dialog semantics and focus management here.
This fullscreen modal is still rendered as plain
divs, so assistive tech never gets a modal dialog announcement and keyboard focus can keep moving through the obscured editor behind it. Please addrole="dialog",aria-modal="true", wirearia-labelledbyto the title, and move focus into the modal on open while keeping focus contained there until close.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/graph/EntityGraphModal.tsx` around lines 43 - 68, The modal currently uses plain divs so screen readers and keyboard users aren't treated as a dialog; update EntityGraphModal to add role="dialog" and aria-modal="true" on the root modal container and add an id to the heading (e.g., heading id like "entity-graph-title") then set aria-labelledby pointing to that id; implement focus management in the EntityGraphModal component so that when opened it saves the previously focused element, moves focus into the modal to a sensible first focusable (e.g., the close button or the heading), traps focus inside the modal (tab/shift-tab cycling) and restores focus on close, and also handle Escape to close the modal. Ensure these changes are hooked into the component lifecycle (useEffect) and reference the modal root and heading/close button refs in the EntityGraphModal implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.planning/features/entity-graph-port/CONTEXT.md:
- Around line 23-27: The API contract in CONTEXT.md still references the old
/concept/{iri_hash}/graph endpoint; update the documentation and examples to the
project-scoped endpoint /api/v1/projects/{id}/graph/entity-graph and the client
call graphApi.getEntityGraph(projectId, focusIri, ...) instead of the old shape,
replace D-05 to show the new GET signature and query params (ancestors_depth,
descendants_depth, max_nodes, include_see_also), remove or mark the old endpoint
as deprecated, and update any downstream PLAN examples or references that copy
the old contract to use the new project-scoped path and the
graphApi.getEntityGraph invocation.
In @.planning/features/entity-graph-port/PLAN.md:
- Around line 76-78: The fenced code blocks showing the example HTTP path and
the plan line are missing a language tag and trigger markdownlint MD040; update
each triple-backtick fence that wraps the lines "GET
/ontologies/{ontology_id}/classes/{class_iri:path}/graph" and "Plan 1 (backend)
→ Plan 2 (frontend components) → Plan 3 (integration)" (and the other occurrence
referenced at lines 238-240) to use a neutral language tag such as ```text so
the blocks become ```text ... ```; do this for all matching fenced blocks in the
file.
In `@lib/hooks/useGraphData.ts`:
- Around line 34-36: The early return in the useGraphData hook when focusIri is
falsy calls setGraphData(null) but does not reset loading state, which can leave
isLoading true; update the branch in the effect that checks focusIri to also
call setIsLoading(false) (alongside setGraphData(null)) before returning so the
hook correctly resets loading when focus is cleared; locate the logic in
useGraphData that references focusIri, setGraphData and setIsLoading and add the
setIsLoading(false) call there.
---
Duplicate comments:
In `@components/graph/EntityGraphModal.tsx`:
- Around line 43-68: The modal currently uses plain divs so screen readers and
keyboard users aren't treated as a dialog; update EntityGraphModal to add
role="dialog" and aria-modal="true" on the root modal container and add an id to
the heading (e.g., heading id like "entity-graph-title") then set
aria-labelledby pointing to that id; implement focus management in the
EntityGraphModal component so that when opened it saves the previously focused
element, moves focus into the modal to a sensible first focusable (e.g., the
close button or the heading), traps focus inside the modal (tab/shift-tab
cycling) and restores focus on close, and also handle Escape to close the modal.
Ensure these changes are hooked into the component lifecycle (useEffect) and
reference the modal root and heading/close button refs in the EntityGraphModal
implementation.
In `@lib/hooks/useGraphData.ts`:
- Around line 84-96: The merged graph object is dropping the truncated flag, so
keep it when merging results in useGraphData (the block that returns {...prev,
nodes: ..., edges: ..., total_concept_count: ...}). Add a truncated property to
the returned object using a boolean union of previous and new results (e.g.,
truncated: prev.truncated || newData.truncated) so any expansion that hit
maxNodes remains marked truncated; reference the variables prev and newData and
the keys nodes, edges, total_concept_count, and truncated when making the
change.
- Around line 106-109: The resetGraph function currently clears expandedNodes
and sets graphData to null, leaving the UI empty; instead restore the baseline
graph by invoking the baseline fetch/refetch logic from this hook: inside
resetGraph (the useCallback) keep expandedNodes.current = new Set(), then call
the existing baseline loader/refetch function (e.g., the BFS initial request
helper or the react-query baseline query's refetch/mutate method used elsewhere
in this hook) rather than only setGraphData(null), ensuring the same code path
that populates graphData on mount runs again; reference the resetGraph callback,
setGraphData, expandedNodes.current, and the baseline BFS/react-query refetch
function so the reset action re-populates the initial graph.
- Around line 27-63: The hook is hand-rolling server state with useEffect and
local state; replace the fetch logic in useGraphData to use React Query's
useQuery for graphApi.getEntityGraph (use a query key that includes projectId,
focusIri, branch and showDescendants), remove manual setIsLoading and graphData
state and consume isLoading/data from useQuery, drop the cancelled cleanup,
initialize expandedNodes.current when focusIri changes (or store it in React
state), and handle expansions as a mutation or by calling
queryClient.setQueryData / invalidateQueries to update the cached query result;
keep references to graphApi.getEntityGraph, useGraphData, expandedNodes,
setShowDescendants and branch when migrating.
- Around line 66-101: The code prematurely calls expandedNodes.current.add(iri)
before the async getEntityGraph response is merged, making failures
non-retryable and allowing late responses to corrupt a different graph; fix by
introducing an in-flight tracker (e.g., inFlightExpanded set) and only add iri
to expandedNodes inside the setGraphData updater after verifying the updater is
merging the response for the current graph instance (i.e., prev is non-null and
matches expected focus/state), remove iri from inFlightExpanded on failure or
after merge, and scope the merge by checking within the setGraphData callback
that the response still applies before updating nodes/edges; update usages
around getEntityGraph, setGraphData, expandedNodes.current.add, and resetGraph
accordingly.
🪄 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
Run ID: 1a1eaaa3-9f2d-4567-8fbe-f76379b6ce60
📒 Files selected for processing (19)
.planning/features/entity-graph-port/CONTEXT.md.planning/features/entity-graph-port/HANDOFF.md.planning/features/entity-graph-port/PLAN.md__tests__/lib/graph/buildGraphData.test.ts__tests__/lib/graph/useELKLayout.test.ts__tests__/lib/graph/utils.test.ts__tests__/lib/hooks/useGraphData.test.tscomponents/editor/developer/DeveloperEditorLayout.tsxcomponents/editor/standard/StandardEditorLayout.tsxcomponents/graph/EntityGraphModal.tsxcomponents/graph/OntologyEdge.tsxcomponents/graph/OntologyGraph.tsxcomponents/graph/OntologyNode.tsxlib/api/graph.tslib/graph/buildGraphData.tslib/graph/elkLayout.tslib/graph/useELKLayout.tslib/graph/utils.tslib/hooks/useGraphData.ts
💤 Files with no reviewable changes (3)
- lib/graph/elkLayout.ts
- tests/lib/graph/buildGraphData.test.ts
- lib/graph/buildGraphData.ts
✅ Files skipped from review due to trivial changes (3)
- tests/lib/graph/utils.test.ts
- components/graph/OntologyEdge.tsx
- .planning/features/entity-graph-port/HANDOFF.md
🚧 Files skipped from review as they are similar to previous changes (8)
- components/graph/OntologyNode.tsx
- components/editor/developer/DeveloperEditorLayout.tsx
- components/editor/standard/StandardEditorLayout.tsx
- tests/lib/hooks/useGraphData.test.ts
- tests/lib/graph/useELKLayout.test.ts
- lib/graph/useELKLayout.ts
- lib/api/graph.ts
- components/graph/OntologyGraph.tsx
59 tasks
JohnRDOrazio
force-pushed
the
entity-graph-pr
branch
from
78beeae to
7a99ad0
Compare
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
.planning/features/entity-graph-port/CONTEXT.md (1)
23-26:⚠️ Potential issue | 🟡 MinorUpdate this API contract to match the implementation.
D-05 still describes
GET /concept/{iri_hash}/graph..., but the shipped client inlib/api/graph.tsnow calls the project-scoped graph route withgraphApi.getEntityGraph(projectId, classIri, ...). Leaving the old path here will send any follow-up work toward the wrong endpoint and parameter contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/features/entity-graph-port/CONTEXT.md around lines 23 - 26, The API contract in D-05 is outdated: it still documents GET /concept/{iri_hash}/graph while the implementation uses a project-scoped route via graphApi.getEntityGraph(projectId, classIri, ...). Update the contract to match the implementation by replacing the old path with the project-scoped endpoint signature (include projectId and classIri/IRI parameter names as used by graphApi.getEntityGraph) and ensure the query params (ancestors_depth, descendants_depth, max_nodes, include_see_also) and the response shape (EntityGraphResponse: nodes, edges, truncated flag, totalConceptCount) match the actual implementation.lib/graph/useELKLayout.ts (1)
27-106:⚠️ Potential issue | 🟠 MajorGuard
runLayout()against stale async completions.Rapid direction changes or graph expansions can leave multiple layouts in flight, and any older completion here can overwrite the newest node/edge state. A simple run-id ref before
setNodes/setEdgeswill keep only the latest layout result.Suggested guard
-import { useCallback, useState } from "react"; +import { useCallback, useRef, useState } from "react"; @@ export function useELKLayout(): LayoutResult { const [nodes, setNodes] = useState<Node<OntologyNodeData>[]>([]); const [edges, setEdges] = useState<Edge<OntologyEdgeData>[]>([]); const [isLayouting, setIsLayouting] = useState(false); + const layoutRunId = useRef(0); @@ const runLayout = useCallback( async (data: EntityGraphResponse, direction: LayoutDirection = "TB") => { + const runId = ++layoutRunId.current; setIsLayouting(true); @@ - setNodes(layoutedNodes); - setEdges(layoutedEdges); + if (runId !== layoutRunId.current) return; + setNodes(layoutedNodes); + setEdges(layoutedEdges); } finally { - setIsLayouting(false); + if (runId === layoutRunId.current) { + setIsLayouting(false); + } } }, [], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/graph/useELKLayout.ts` around lines 27 - 106, runLayout can have multiple in-flight async runs that overwrite newer results; add a numeric run-id guard using a useRef (e.g., layoutRunIdRef) incremented at start of runLayout and captured in the async closure, then before calling setNodes, setEdges (and setIsLayouting(false)) verify the captured id matches layoutRunIdRef.current so only the latest layout applies; create layoutRunIdRef alongside the hook state, increment it at the top of runLayout, capture its value in a local const runId, and conditionally apply state updates only when runId === layoutRunIdRef.current.lib/hooks/useGraphData.ts (5)
84-96:⚠️ Potential issue | 🟠 MajorPreserve
truncatedwhen expansion results are merged.The merged object keeps
prev.truncated, so any expansion response that hitsmaxNodeswill never surface the truncation badge in the UI.Minimal fix
return { ...prev, + truncated: prev.truncated || newData.truncated, nodes: [ ...prev.nodes, ...newData.nodes.filter((n) => !existingNodeIds.has(n.id)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useGraphData.ts` around lines 84 - 96, When merging expansion results in useGraphData.ts (the return object that spreads prev and appends to nodes and edges), preserve the truncation state by setting truncated to prev.truncated || newData.truncated instead of keeping only prev.truncated; update the merged object (alongside nodes, edges, total_concept_count) to include truncated: prev.truncated || newData.truncated so the UI shows the truncation badge when any expansion hit maxNodes.
27-63:⚠️ Potential issue | 🟠 MajorMove the baseline graph fetch to React Query.
This hook is still hand-rolling server-state management with
useEffect+ local state, so cache/dedup/refetch/error semantics are being reimplemented here and are already leaking into bugs likeresetGraph(). This should be auseQuerykeyed byprojectId,focusIri,branch, andshowDescendants, with the hook readingdata/isLoading/errorfrom React Query instead of maintaining them manually. As per coding guidelines, "Use React Query (@tanstack/react-query) for server state management".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useGraphData.ts` around lines 27 - 63, Replace the manual useEffect fetch in the useGraphData hook with a React Query useQuery: call graphApi.getEntityGraph inside useQuery keyed by ['entityGraph', projectId, focusIri, branch, showDescendants] and set enabled: !!focusIri; map the query's data/isLoading/error to the hook's returned graphData/isLoading/error instead of maintaining local fetching state and cancellation flags; use useQuery's onSuccess to set expandedNodes.current = new Set([focusIri]) (or update it when data arrives) and optionally keepPreviousData/refetch options as needed; remove the existing useEffect, cancelled flag, and manual setIsLoading/setGraphData logic so React Query owns caching/dedup/refetch behavior.
106-109:⚠️ Potential issue | 🟠 Major
resetGraph()clears the graph but never restores the baseline data.This only wipes local state. Because no fetch dependency changes here, the initial BFS request is not re-run, so reset leaves the hook empty until some unrelated prop changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useGraphData.ts` around lines 106 - 109, resetGraph currently only clears expandedNodes and nulls graph state, leaving the hook empty because the initial BFS/fetch is not re-triggered; modify resetGraph to restore the baseline graph instead of nulling it by setting setGraphData to the stored initial/baseline graph (e.g. an existing ref like initialGraphRef.current or baselineGraph) or by calling the refetch function that triggers the initial BFS (e.g. fetchGraphBFS()/refetchGraph) so the hook returns to its original state; keep the expandedNodes.current = new Set() behavior and update references to resetGraph, expandedNodes, and setGraphData accordingly.
34-36:⚠️ Potential issue | 🟡 MinorClear loading state when the focus is removed.
If
focusIribecomesnullwhile a request is in flight, this branch leavesisLoadingstucktruebecause the cancelled request'sfinallyalso stops updating it.Minimal fix
if (!focusIri) { + setIsLoading(false); setGraphData(null); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useGraphData.ts` around lines 34 - 36, When focusIri becomes null we currently setGraphData(null) and return but leave isLoading true if a request is in flight; update the useGraphData hook to (1) setIsLoading(false) before returning in the branch that handles !focusIri and (2) cancel any pending request so its finally block cannot overwrite state — e.g. call abort() on the AbortController or mark/clear the currentRequestRef used by the fetch logic; reference setGraphData, isLoading, and the hook’s request controller/ref (the variable managing the in-flight request) when making the change.
67-101:⚠️ Potential issue | 🟠 MajorDon't promote a node to expanded before its fetch is applied.
expandedNodes.current.add(iri)happens before the request succeeds, which makes transient failures non-retryable and lets late responses mutate a graph that has already been reset or replaced. Track an in-flight set separately and only move the node intoexpandedNodesafter the merge is accepted for the current graph instance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useGraphData.ts` around lines 67 - 101, The code currently calls expandedNodes.current.add(iri) before the network request completes; instead add an in-flight tracker (e.g., inFlightExpansions.current) and only mark a node expanded after the merge actually updates graph state inside the setGraphData functional updater. Concretely: before calling graphApi.getEntityGraph, add iri to inFlightExpansions.current to prevent duplicate requests; on success, call setGraphData(prev => { if (!prev) { const merged = newData; /* return merged */ } else { compute merged by filtering existingNodeIds/existingEdgeIds } return merged; }) and inside that success handler (after computing/returning merged) add iri to expandedNodes.current and remove from inFlightExpansions.current; on catch remove iri from inFlightExpansions.current and avoid adding to expandedNodes. Use the identifiers expandedNodes.current, inFlightExpansions.current, graphApi.getEntityGraph, and setGraphData to locate where to implement this.components/graph/OntologyGraph.tsx (1)
187-196:⚠️ Potential issue | 🟠 MajorKeep navigation on double-click only.
Because
expandedNodesis seeded withfocusIri, the focus node already navigates on the first single click, and any expanded node keeps doing the same. That breaks the intended click-to-expand / double-click-to-navigate behavior and can close the modal on a single click.Minimal fix
const handleNodeClick: NodeMouseHandler = useCallback( (_event, node) => { - if (expandedNodes.has(node.id)) { - onNavigateToClass?.(node.id); - return; - } + if (expandedNodes.has(node.id)) return; expandedNodes.add(node.id); expandNode(node.id); }, - [expandedNodes, expandNode, onNavigateToClass], + [expandedNodes, expandNode], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/graph/OntologyGraph.tsx` around lines 187 - 196, The single-click handler handleNodeClick currently calls onNavigateToClass when expandedNodes.has(node.id), causing navigation on single click; change handleNodeClick so it never calls onNavigateToClass (only toggle/expand behaviour: if already expanded do nothing or prevent re-opening, otherwise add to expandedNodes and call expandNode(node.id)), and move the navigation call to the double-click handler (e.g., implement or update handleNodeDoubleClick to call onNavigateToClass(node.id)). Adjust hooks/deps (useCallback dependencies for handleNodeClick and the double-click handler) and ensure expandedNodes seeding with focusIri no longer triggers single-click navigation..planning/features/entity-graph-port/PLAN.md (1)
76-78:⚠️ Potential issue | 🟡 MinorAdd language tags to these fenced blocks.
Both fences are still going to trip markdownlint MD040 until they become something like
```text.Also applies to: 238-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/features/entity-graph-port/PLAN.md around lines 76 - 78, The fenced code blocks containing the endpoint examples (e.g., the block with "GET /ontologies/{ontology_id}/classes/{class_iri:path}/graph" and the other similar block later) need an explicit language tag to satisfy markdownlint MD040; update each triple-backtick fence to include a language identifier such as text (e.g., change ``` to ```text) so the blocks are language-annotated while leaving the content unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.planning/features/entity-graph-port/PLAN.md:
- Around line 74-80: Update the PLAN.md to match the shipped project-scoped
graph API: replace the old route and parameter names (the GET
/ontologies/{ontology_id}/classes/{class_iri:path}/graph and any use of
ontologyId) with the project-scoped variant used by the implementation (the
graphApi.getEntityGraph(projectId, classIri, ...) contract), ensure example
endpoint paths, parameter names (projectId, entity/class IRI) and query params
match lib/api/graph.ts, and make the same replacements in the other referenced
sections so all examples and docs point to the project-scoped API rather than
the superseded ontology-scoped route.
In `@components/graph/OntologyGraph.tsx`:
- Around line 176-182: The useEffect that syncs layout results only updates
React Flow when layoutNodes.length > 0, which leaves stale nodes/edges when
graphData becomes null/empty; modify the effect in OntologyGraph (the useEffect
that references layoutNodes, layoutEdges, setNodes, setEdges) to setNodes([])
and setEdges([]) when layoutNodes is empty (or graphData is null/empty) so the
React Flow state is cleared instead of retaining the previous graph.
In `@lib/graph/useELKLayout.ts`:
- Around line 76-79: The code in useELKLayout currently casts
backendNode.node_type into GraphNodeType which ignores the new "secondary_root"
value; instead, explicitly detect backendNode.node_type === "secondary_root" (or
backendNode.secondary_root flag if present) and map it to the appropriate
GraphNodeType/variant (or extend GraphNodeType union) before assigning to
nodeType so the renderer/MiniMap receive a normalized value (e.g., map
"secondary_root" -> "ancestor" or a new "secondary_root" type) and update any
downstream styling logic that checks GraphNodeType accordingly.
---
Duplicate comments:
In @.planning/features/entity-graph-port/CONTEXT.md:
- Around line 23-26: The API contract in D-05 is outdated: it still documents
GET /concept/{iri_hash}/graph while the implementation uses a project-scoped
route via graphApi.getEntityGraph(projectId, classIri, ...). Update the contract
to match the implementation by replacing the old path with the project-scoped
endpoint signature (include projectId and classIri/IRI parameter names as used
by graphApi.getEntityGraph) and ensure the query params (ancestors_depth,
descendants_depth, max_nodes, include_see_also) and the response shape
(EntityGraphResponse: nodes, edges, truncated flag, totalConceptCount) match the
actual implementation.
In @.planning/features/entity-graph-port/PLAN.md:
- Around line 76-78: The fenced code blocks containing the endpoint examples
(e.g., the block with "GET
/ontologies/{ontology_id}/classes/{class_iri:path}/graph" and the other similar
block later) need an explicit language tag to satisfy markdownlint MD040; update
each triple-backtick fence to include a language identifier such as text (e.g.,
change ``` to ```text) so the blocks are language-annotated while leaving the
content unchanged.
In `@components/graph/OntologyGraph.tsx`:
- Around line 187-196: The single-click handler handleNodeClick currently calls
onNavigateToClass when expandedNodes.has(node.id), causing navigation on single
click; change handleNodeClick so it never calls onNavigateToClass (only
toggle/expand behaviour: if already expanded do nothing or prevent re-opening,
otherwise add to expandedNodes and call expandNode(node.id)), and move the
navigation call to the double-click handler (e.g., implement or update
handleNodeDoubleClick to call onNavigateToClass(node.id)). Adjust hooks/deps
(useCallback dependencies for handleNodeClick and the double-click handler) and
ensure expandedNodes seeding with focusIri no longer triggers single-click
navigation.
In `@lib/graph/useELKLayout.ts`:
- Around line 27-106: runLayout can have multiple in-flight async runs that
overwrite newer results; add a numeric run-id guard using a useRef (e.g.,
layoutRunIdRef) incremented at start of runLayout and captured in the async
closure, then before calling setNodes, setEdges (and setIsLayouting(false))
verify the captured id matches layoutRunIdRef.current so only the latest layout
applies; create layoutRunIdRef alongside the hook state, increment it at the top
of runLayout, capture its value in a local const runId, and conditionally apply
state updates only when runId === layoutRunIdRef.current.
In `@lib/hooks/useGraphData.ts`:
- Around line 84-96: When merging expansion results in useGraphData.ts (the
return object that spreads prev and appends to nodes and edges), preserve the
truncation state by setting truncated to prev.truncated || newData.truncated
instead of keeping only prev.truncated; update the merged object (alongside
nodes, edges, total_concept_count) to include truncated: prev.truncated ||
newData.truncated so the UI shows the truncation badge when any expansion hit
maxNodes.
- Around line 27-63: Replace the manual useEffect fetch in the useGraphData hook
with a React Query useQuery: call graphApi.getEntityGraph inside useQuery keyed
by ['entityGraph', projectId, focusIri, branch, showDescendants] and set
enabled: !!focusIri; map the query's data/isLoading/error to the hook's returned
graphData/isLoading/error instead of maintaining local fetching state and
cancellation flags; use useQuery's onSuccess to set expandedNodes.current = new
Set([focusIri]) (or update it when data arrives) and optionally
keepPreviousData/refetch options as needed; remove the existing useEffect,
cancelled flag, and manual setIsLoading/setGraphData logic so React Query owns
caching/dedup/refetch behavior.
- Around line 106-109: resetGraph currently only clears expandedNodes and nulls
graph state, leaving the hook empty because the initial BFS/fetch is not
re-triggered; modify resetGraph to restore the baseline graph instead of nulling
it by setting setGraphData to the stored initial/baseline graph (e.g. an
existing ref like initialGraphRef.current or baselineGraph) or by calling the
refetch function that triggers the initial BFS (e.g.
fetchGraphBFS()/refetchGraph) so the hook returns to its original state; keep
the expandedNodes.current = new Set() behavior and update references to
resetGraph, expandedNodes, and setGraphData accordingly.
- Around line 34-36: When focusIri becomes null we currently setGraphData(null)
and return but leave isLoading true if a request is in flight; update the
useGraphData hook to (1) setIsLoading(false) before returning in the branch that
handles !focusIri and (2) cancel any pending request so its finally block cannot
overwrite state — e.g. call abort() on the AbortController or mark/clear the
currentRequestRef used by the fetch logic; reference setGraphData, isLoading,
and the hook’s request controller/ref (the variable managing the in-flight
request) when making the change.
- Around line 67-101: The code currently calls expandedNodes.current.add(iri)
before the network request completes; instead add an in-flight tracker (e.g.,
inFlightExpansions.current) and only mark a node expanded after the merge
actually updates graph state inside the setGraphData functional updater.
Concretely: before calling graphApi.getEntityGraph, add iri to
inFlightExpansions.current to prevent duplicate requests; on success, call
setGraphData(prev => { if (!prev) { const merged = newData; /* return merged */
} else { compute merged by filtering existingNodeIds/existingEdgeIds } return
merged; }) and inside that success handler (after computing/returning merged)
add iri to expandedNodes.current and remove from inFlightExpansions.current; on
catch remove iri from inFlightExpansions.current and avoid adding to
expandedNodes. Use the identifiers expandedNodes.current,
inFlightExpansions.current, graphApi.getEntityGraph, and setGraphData to locate
where to implement this.
🪄 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
Run ID: 4678a87f-048a-4b03-a9b5-9a54ebf998b4
📒 Files selected for processing (19)
.planning/features/entity-graph-port/CONTEXT.md.planning/features/entity-graph-port/HANDOFF.md.planning/features/entity-graph-port/PLAN.md__tests__/lib/graph/buildGraphData.test.ts__tests__/lib/graph/useELKLayout.test.ts__tests__/lib/graph/utils.test.ts__tests__/lib/hooks/useGraphData.test.tscomponents/editor/developer/DeveloperEditorLayout.tsxcomponents/editor/standard/StandardEditorLayout.tsxcomponents/graph/EntityGraphModal.tsxcomponents/graph/OntologyEdge.tsxcomponents/graph/OntologyGraph.tsxcomponents/graph/OntologyNode.tsxlib/api/graph.tslib/graph/buildGraphData.tslib/graph/elkLayout.tslib/graph/useELKLayout.tslib/graph/utils.tslib/hooks/useGraphData.ts
💤 Files with no reviewable changes (3)
- lib/graph/elkLayout.ts
- tests/lib/graph/buildGraphData.test.ts
- lib/graph/buildGraphData.ts
✅ Files skipped from review due to trivial changes (5)
- tests/lib/graph/utils.test.ts
- components/graph/OntologyNode.tsx
- components/graph/OntologyEdge.tsx
- .planning/features/entity-graph-port/HANDOFF.md
- lib/api/graph.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- components/editor/developer/DeveloperEditorLayout.tsx
- lib/graph/utils.ts
- components/editor/standard/StandardEditorLayout.tsx
- tests/lib/hooks/useGraphData.test.ts
- components/graph/EntityGraphModal.tsx
JohnRDOrazio
force-pushed
the
entity-graph-pr
branch
from
a6356aa to
ffbfee7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
lib/hooks/useGraphData.ts (2)
71-104:⚠️ Potential issue | 🟠 MajorDiscard expansion responses from older graph instances.
If focus, branch, descendants mode, or a reset changes the base graph while this request is in flight, the stale
.then()still mutates whatever graph is current when it resolves. In the!prevpath it can even repopulate a freshly reset view with an unrelated node slice. Gate the merge with a graph-instance token before callingsetGraphDataor marking the node expanded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useGraphData.ts` around lines 71 - 104, Before calling graphApi.getEntityGraph, capture the current graph-instance token (e.g., graphInstanceTokenRef.current) into a local const and, inside the .then() and .catch() handlers, first check that the captured token still equals the live token; only if it matches perform expandedNodes.current.add(iri) and call setGraphData (or mutate state). Apply the same token check in the !prev branch to avoid repopulating a reset view; if the tokens differ simply ignore the response. Ensure the token variable is unique and referenced alongside graphApi.getEntityGraph, expandedNodes.current.add, and setGraphData so stale responses are discarded.
27-64: 🛠️ Refactor suggestion | 🟠 MajorMove the baseline graph fetch into React Query.
This hook is still hand-rolling server-state lifecycle with
useEffect/useState, so it bypasses the repo’s cache/dedup/retry conventions and keeps reimplementing fetch bookkeeping locally.As per coding guidelines, "Use React Query (
@tanstack/react-query) for server state management."#!/bin/bash # Compare this hook with the repo's existing React Query patterns. sed -n '1,140p' lib/hooks/useGraphData.ts printf '\n--- lib/hooks/useEntityHistory.ts ---\n' sed -n '1,140p' lib/hooks/useEntityHistory.ts printf '\n--- lib/hooks/useCrossReferences.ts ---\n' sed -n '1,140p' lib/hooks/useCrossReferences.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useGraphData.ts` around lines 27 - 64, Replace the manual useEffect/useState fetch in the useGraphData hook with a React Query useQuery: call useQuery with a key like ['entityGraph', projectId, focusIri, branch, showDescendants, resetKey], a fetcher that calls graphApi.getEntityGraph(projectId, focusIri, { branch, ancestorsDepth: 5, descendantsDepth: showDescendants ? 2 : 0 }), and enabled: !!focusIri to avoid running when focusIri is falsy; remove local isLoading/graphData state and instead expose query.data and query.isLoading, use the query's onSuccess to set expandedNodes.current = new Set([focusIri]) (and any other side-effects), and keep resetKey in the query key to allow manual refetching; ensure error handling relies on React Query (remove the .catch/.finally blocks and local cancelled flag).components/graph/OntologyGraph.tsx (1)
168-180:⚠️ Potential issue | 🟠 MajorClear React Flow state when there’s no graph to lay out.
When
graphDatabecomesnullor empty after reset or fetch failure,runLayout()exits early and the previouslayoutNodes/layoutEdgesstay rendered. The toolbar can show0 nodes, 0 edgeswhile the old graph is still on screen.Suggested fix
useEffect(() => { + if (!graphData || graphData.nodes.length === 0) { + setNodes([]); + setEdges([]); + return; + } setNodes(layoutNodes); setEdges(layoutEdges); - }, [layoutNodes, layoutEdges, setNodes, setEdges]); + }, [graphData, layoutNodes, layoutEdges, setNodes, setEdges]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/graph/OntologyGraph.tsx` around lines 168 - 180, When graphData becomes null/empty the first useEffect currently returns early leaving previous layoutNodes/layoutEdges rendered; update the early-return branch in the effect that calls runLayout(graphData, direction) to proactively clear React Flow state by calling setNodes([]) and setEdges([]) (or otherwise reset layoutNodes/layoutEdges) when graphData is falsy or graphData.nodes.length === 0, so the UI and toolbar reflect the empty graph; keep the existing runLayout.catch handling but ensure you clear state on both the empty-data path and on layout failure as needed.lib/graph/useELKLayout.ts (1)
78-84:⚠️ Potential issue | 🟠 MajorPreserve
secondary_rootinstead of casting it away.The backend contract now distinguishes
rootandsecondary_root, but this cast pushes every backend string throughGraphNodeTypewithout normalization.secondary_rootwill fall through renderer/MiniMap default styling instead of getting the dedicated secondary-root treatment described in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/graph/useELKLayout.ts` around lines 78 - 84, backendNode.node_type is being force-cast to GraphNodeType which drops the distinct "secondary_root" value; instead preserve that value so the renderer/MiniMap can style it. Update the data construction (the object with label, nodeType, childCount, deprecated, isExpanded) to either set nodeType = backendNode.node_type ?? "class" without an unsafe cast or add an explicit flag (e.g., secondaryRoot = backendNode.node_type === "secondary_root") so "secondary_root" is preserved and can be detected by the renderer/MiniMap; reference backendNode, node_type, GraphNodeType, and the data object when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/graph/OntologyGraph.tsx`:
- Around line 183-188: expandedNodes.current is only reset when focusIri
changes, so when the base graph is rebuilt (e.g., after Reset or toggling
descendants) stale ids remain expanded and clicks become no-ops; update the
useEffect that sets expandedNodes.current to also depend on the graph-rebuild
signal (for example a prop or state like graphVersion, rootIri, baseGraph, or
resetCounter) and clear/reinitialize expandedNodes.current there (do the same
change for the other similar effect around the expanded-node handling at the
later block referenced). Locate the useEffect that currently lists [focusIri]
and add the appropriate rebuild identifier to its dependency array, and ensure
expandedNodes.current = new Set<string>(focusIri ? [focusIri] : []) runs when
the graph is rebuilt.
---
Duplicate comments:
In `@components/graph/OntologyGraph.tsx`:
- Around line 168-180: When graphData becomes null/empty the first useEffect
currently returns early leaving previous layoutNodes/layoutEdges rendered;
update the early-return branch in the effect that calls runLayout(graphData,
direction) to proactively clear React Flow state by calling setNodes([]) and
setEdges([]) (or otherwise reset layoutNodes/layoutEdges) when graphData is
falsy or graphData.nodes.length === 0, so the UI and toolbar reflect the empty
graph; keep the existing runLayout.catch handling but ensure you clear state on
both the empty-data path and on layout failure as needed.
In `@lib/graph/useELKLayout.ts`:
- Around line 78-84: backendNode.node_type is being force-cast to GraphNodeType
which drops the distinct "secondary_root" value; instead preserve that value so
the renderer/MiniMap can style it. Update the data construction (the object with
label, nodeType, childCount, deprecated, isExpanded) to either set nodeType =
backendNode.node_type ?? "class" without an unsafe cast or add an explicit flag
(e.g., secondaryRoot = backendNode.node_type === "secondary_root") so
"secondary_root" is preserved and can be detected by the renderer/MiniMap;
reference backendNode, node_type, GraphNodeType, and the data object when making
the change.
In `@lib/hooks/useGraphData.ts`:
- Around line 71-104: Before calling graphApi.getEntityGraph, capture the
current graph-instance token (e.g., graphInstanceTokenRef.current) into a local
const and, inside the .then() and .catch() handlers, first check that the
captured token still equals the live token; only if it matches perform
expandedNodes.current.add(iri) and call setGraphData (or mutate state). Apply
the same token check in the !prev branch to avoid repopulating a reset view; if
the tokens differ simply ignore the response. Ensure the token variable is
unique and referenced alongside graphApi.getEntityGraph,
expandedNodes.current.add, and setGraphData so stale responses are discarded.
- Around line 27-64: Replace the manual useEffect/useState fetch in the
useGraphData hook with a React Query useQuery: call useQuery with a key like
['entityGraph', projectId, focusIri, branch, showDescendants, resetKey], a
fetcher that calls graphApi.getEntityGraph(projectId, focusIri, { branch,
ancestorsDepth: 5, descendantsDepth: showDescendants ? 2 : 0 }), and enabled:
!!focusIri to avoid running when focusIri is falsy; remove local
isLoading/graphData state and instead expose query.data and query.isLoading, use
the query's onSuccess to set expandedNodes.current = new Set([focusIri]) (and
any other side-effects), and keep resetKey in the query key to allow manual
refetching; ensure error handling relies on React Query (remove the
.catch/.finally blocks and local cancelled flag).
🪄 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
Run ID: dc83f0ba-baf4-4146-b203-8772ac060a8e
📒 Files selected for processing (24)
.planning/features/entity-graph-port/CONTEXT.md.planning/features/entity-graph-port/HANDOFF.md.planning/features/entity-graph-port/PLAN.md__tests__/components/graph/EntityGraphModal.test.tsx__tests__/components/graph/OntologyEdge.test.tsx__tests__/components/graph/OntologyGraph.test.tsx__tests__/lib/api/graph.test.ts__tests__/lib/graph/buildGraphData.test.ts__tests__/lib/graph/elkLayout.test.ts__tests__/lib/graph/useELKLayout.test.ts__tests__/lib/graph/utils.test.ts__tests__/lib/hooks/useGraphData.test.tscomponents/editor/developer/DeveloperEditorLayout.tsxcomponents/editor/standard/StandardEditorLayout.tsxcomponents/graph/EntityGraphModal.tsxcomponents/graph/OntologyEdge.tsxcomponents/graph/OntologyGraph.tsxcomponents/graph/OntologyNode.tsxlib/api/graph.tslib/graph/buildGraphData.tslib/graph/elkLayout.tslib/graph/useELKLayout.tslib/graph/utils.tslib/hooks/useGraphData.ts
💤 Files with no reviewable changes (4)
- tests/lib/graph/elkLayout.test.ts
- lib/graph/elkLayout.ts
- tests/lib/graph/buildGraphData.test.ts
- lib/graph/buildGraphData.ts
✅ Files skipped from review due to trivial changes (7)
- tests/components/graph/OntologyEdge.test.tsx
- components/graph/OntologyEdge.tsx
- tests/lib/graph/utils.test.ts
- components/graph/OntologyNode.tsx
- .planning/features/entity-graph-port/PLAN.md
- lib/api/graph.ts
- .planning/features/entity-graph-port/HANDOFF.md
🚧 Files skipped from review as they are similar to previous changes (5)
- components/editor/developer/DeveloperEditorLayout.tsx
- lib/graph/utils.ts
- components/graph/EntityGraphModal.tsx
- tests/lib/hooks/useGraphData.test.ts
- .planning/features/entity-graph-port/CONTEXT.md
Member
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/hooks/useGraphData.ts (1)
27-64: 🛠️ Refactor suggestion | 🟠 MajorMigrate to React Query for server state management.
This hook manually implements server-state management with
useEffect+useStateand cancellation logic. Per coding guidelines, server state should useuseQueryfrom@tanstack/react-query. This would provide automatic caching, deduplication, loading/error states, and retry logic. See existing hooks likeuseEntityHistory.tsanduseCrossReferences.tsfor the established pattern.As per coding guidelines: "Use React Query (
@tanstack/react-query) for server state management".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useGraphData.ts` around lines 27 - 64, Replace the manual useEffect/useState server-fetch in useGraphData (the graphApi.getEntityGraph call and the related state variables graphData/isLoading/resetKey and cancellation logic) with a React Query useQuery hook wired to projectId, focusIri, branch, showDescendants (useQuery key should include these values and resetKey if still needed) and return query's data/isLoading/error instead of managing setGraphData/setIsLoading; keep expandedNodes as a ref, and follow the pattern used in useEntityHistory.ts/useCrossReferences.ts (use graphApi.getEntityGraph as the fetcher and set descendantsDepth based on showDescendants).
🧹 Nitpick comments (2)
__tests__/lib/hooks/useGraphData.test.ts (1)
304-308: Synchronous assertion after async expansion may be fragile.The second
expandNodecall is expected to be skipped synchronously because the node is already inexpandedNodes. However, since the first expansion's.then()handler adds toexpandedNodes, and that happens asynchronously, there's a potential race if thewaitForon line 300-302 resolves before the mock is fully processed. The test currently works becausemockResolvedValueOnceresolves synchronously in the test environment, but this coupling is implicit.Consider adding a brief comment explaining why this synchronous check is valid, or wrapping the second call in a small delay/waitFor for robustness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useGraphData.test.ts` around lines 304 - 308, The synchronous assertion is fragile because expandNode("urn:parent") relies on the prior async resolution that updates expandedNodes; update the test around the second expand to ensure robustness by either adding a brief explanatory comment referencing expandNode and expandedNodes to justify the synchronous skip, or change the second call to be checked inside a waitFor/after a microtask tick so the first mock resolution has been processed before asserting mockGetEntityGraph call count (i.e., locate the second expandNode call and the expect against mockGetEntityGraph and wrap or await a short waitFor/Promise.resolve().then() before asserting).components/graph/EntityGraphModal.tsx (1)
51-68: Minor edge case: single focusable element.When there's only one focusable element (
focusable.length === 1),firstandlastare the same element. The Tab trap still works correctly in this case, but it might be worth adding a comment for clarity or explicitly handling the single-element case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/graph/EntityGraphModal.tsx` around lines 51 - 68, The Tab-focus trap in EntityGraphModal's key handler can be clearer for the single-focusable-element case: explicitly check for focusable.length === 1 (using dialogRef.current.querySelectorAll result stored in focusable) and handle it by preventing default Tab behavior and keeping focus on that single element (or add a concise comment explaining that first === last when length === 1 and that the current logic already covers it); update the block around dialogRef, focusable, first, and last to either handle length === 1 explicitly or add the clarifying comment so future readers understand the edge case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/hooks/useGraphData.ts`:
- Around line 97-98: The merge logic incorrectly adds deduplicated new nodes to
prev.total_concept_count; instead, update total_concept_count to reflect the
backend-provided fixed subgraph count by replacing the addition with
Math.max(prev.total_concept_count, newData.total_concept_count) (or simply keep
prev.total_concept_count if you prefer preserving original); change the
expression that references total_concept_count in useGraphData.ts (the block
using prev, newData, existingNodeIds and nodes) to use
Math.max(prev.total_concept_count, newData.total_concept_count) so the merged
value is not an incorrect running tally.
---
Duplicate comments:
In `@lib/hooks/useGraphData.ts`:
- Around line 27-64: Replace the manual useEffect/useState server-fetch in
useGraphData (the graphApi.getEntityGraph call and the related state variables
graphData/isLoading/resetKey and cancellation logic) with a React Query useQuery
hook wired to projectId, focusIri, branch, showDescendants (useQuery key should
include these values and resetKey if still needed) and return query's
data/isLoading/error instead of managing setGraphData/setIsLoading; keep
expandedNodes as a ref, and follow the pattern used in
useEntityHistory.ts/useCrossReferences.ts (use graphApi.getEntityGraph as the
fetcher and set descendantsDepth based on showDescendants).
---
Nitpick comments:
In `@__tests__/lib/hooks/useGraphData.test.ts`:
- Around line 304-308: The synchronous assertion is fragile because
expandNode("urn:parent") relies on the prior async resolution that updates
expandedNodes; update the test around the second expand to ensure robustness by
either adding a brief explanatory comment referencing expandNode and
expandedNodes to justify the synchronous skip, or change the second call to be
checked inside a waitFor/after a microtask tick so the first mock resolution has
been processed before asserting mockGetEntityGraph call count (i.e., locate the
second expandNode call and the expect against mockGetEntityGraph and wrap or
await a short waitFor/Promise.resolve().then() before asserting).
In `@components/graph/EntityGraphModal.tsx`:
- Around line 51-68: The Tab-focus trap in EntityGraphModal's key handler can be
clearer for the single-focusable-element case: explicitly check for
focusable.length === 1 (using dialogRef.current.querySelectorAll result stored
in focusable) and handle it by preventing default Tab behavior and keeping focus
on that single element (or add a concise comment explaining that first === last
when length === 1 and that the current logic already covers it); update the
block around dialogRef, focusable, first, and last to either handle length === 1
explicitly or add the clarifying comment so future readers understand the edge
case.
🪄 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
Run ID: f4096ff3-a001-4088-a0cb-50b3fab28049
📒 Files selected for processing (24)
.planning/features/entity-graph-port/CONTEXT.md.planning/features/entity-graph-port/HANDOFF.md.planning/features/entity-graph-port/PLAN.md__tests__/components/graph/EntityGraphModal.test.tsx__tests__/components/graph/OntologyEdge.test.tsx__tests__/components/graph/OntologyGraph.test.tsx__tests__/lib/api/graph.test.ts__tests__/lib/graph/buildGraphData.test.ts__tests__/lib/graph/elkLayout.test.ts__tests__/lib/graph/useELKLayout.test.ts__tests__/lib/graph/utils.test.ts__tests__/lib/hooks/useGraphData.test.tscomponents/editor/developer/DeveloperEditorLayout.tsxcomponents/editor/standard/StandardEditorLayout.tsxcomponents/graph/EntityGraphModal.tsxcomponents/graph/OntologyEdge.tsxcomponents/graph/OntologyGraph.tsxcomponents/graph/OntologyNode.tsxlib/api/graph.tslib/graph/buildGraphData.tslib/graph/elkLayout.tslib/graph/useELKLayout.tslib/graph/utils.tslib/hooks/useGraphData.ts
💤 Files with no reviewable changes (4)
- tests/lib/graph/elkLayout.test.ts
- lib/graph/elkLayout.ts
- lib/graph/buildGraphData.ts
- tests/lib/graph/buildGraphData.test.ts
✅ Files skipped from review due to trivial changes (10)
- tests/components/graph/OntologyEdge.test.tsx
- components/graph/OntologyEdge.tsx
- components/graph/OntologyNode.tsx
- tests/lib/graph/utils.test.ts
- lib/graph/utils.ts
- tests/lib/api/graph.test.ts
- .planning/features/entity-graph-port/PLAN.md
- .planning/features/entity-graph-port/HANDOFF.md
- tests/lib/graph/useELKLayout.test.ts
- lib/api/graph.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- components/editor/developer/DeveloperEditorLayout.tsx
- tests/components/graph/EntityGraphModal.test.tsx
- .planning/features/entity-graph-port/CONTEXT.md
- components/graph/OntologyGraph.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
lib/hooks/useGraphData.ts (2)
78-82:⚠️ Potential issue | 🟠 MajorScope expansion responses to the active graph instance.
An in-flight
expandNode()response can still apply after a focus/reset change (Line 109-113), because the merge callback does not validate request generation. This can inject stale subgraphs into the new graph state.Suggested hardening
+ const graphGeneration = useRef(0); + useEffect(() => { + graphGeneration.current += 1; if (!focusIri) { setGraphData(null); setIsLoading(false); return; } @@ const expandNode = useCallback( (iri: string) => { if (!graphData || expandedNodes.current.has(iri)) return; + const generationAtStart = graphGeneration.current; @@ .then((newData) => { - expandedNodes.current.add(iri); setGraphData((prev) => { + if (graphGeneration.current !== generationAtStart) return prev; if (!prev) return newData; @@ }); + expandedNodes.current.add(iri); })Also applies to: 109-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useGraphData.ts` around lines 78 - 82, The merge callback for expandNode can apply stale results to a new graph because it doesn't validate that the response belongs to the current graph/focus; fix expandNode by stamping each request with a unique id or token (e.g., requestIdRef or generationRef) before the async fetch, capture that id in the .then handler, and before calling expandedNodes.current.add(iri) and setGraphData merge callback verify the captured id matches the current id (or that the request wasn't aborted); if it doesn't match, discard the response. Ensure this check is applied both where expandedNodes.current is mutated and where setGraphData's merge logic runs so stale subgraphs are never injected.
34-64:⚠️ Potential issue | 🟠 MajorMigrate baseline graph fetch to React Query.
This hook still manages server state with
useEffect/useStateinstead ofuseQuery, which is inconsistent with the repo standard and loses built-in cache/dedup/retry semantics.#!/bin/bash set -euo pipefail echo "Inspect useGraphData fetch implementation:" cat -n lib/hooks/useGraphData.ts | sed -n '30,90p' echo echo "Compare with React Query usage in other hooks:" rg -n --type ts --type tsx 'from "@tanstack/react-query"|useQuery\(' lib/hooksAs per coding guidelines, "Use React Query (
@tanstack/react-query) for server state management."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useGraphData.ts` around lines 34 - 64, Replace the manual useEffect/useState fetch with a React Query query: remove the effect that calls graphApi.getEntityGraph and the cancelled flag, and instead create a useQuery keyed by [ "entityGraph", projectId, focusIri, branch, showDescendants, resetKey ] that calls graphApi.getEntityGraph(projectId, focusIri, { branch, ancestorsDepth: 5, descendantsDepth: showDescendants ? 2 : 0 }); map the query's data/isLoading/error to the existing state handlers (setGraphData, setIsLoading) or eliminate local state in favor of query data/isLoading, and keep expandedNodes.current initialization when focusIri changes (e.g., in a lightweight effect) so existing behavior around expandedNodes.current = new Set([focusIri]) remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/hooks/useGraphData.ts`:
- Around line 45-50: The graph API calls in useGraphData are missing auth:
update graphApi.getEntityGraph to accept an optional token?: string parameter
and forward it as headers: token ? { Authorization: `Bearer ${token}` } :
undefined when making the HTTP request, then update useGraphData to accept an
accessToken prop/arg and pass it into graphApi.getEntityGraph calls (both the
focusIri call and the fallback call) so the Authorization header is applied like
other project-scoped endpoints; reference graphApi.getEntityGraph and
useGraphData when making these changes.
---
Duplicate comments:
In `@lib/hooks/useGraphData.ts`:
- Around line 78-82: The merge callback for expandNode can apply stale results
to a new graph because it doesn't validate that the response belongs to the
current graph/focus; fix expandNode by stamping each request with a unique id or
token (e.g., requestIdRef or generationRef) before the async fetch, capture that
id in the .then handler, and before calling expandedNodes.current.add(iri) and
setGraphData merge callback verify the captured id matches the current id (or
that the request wasn't aborted); if it doesn't match, discard the response.
Ensure this check is applied both where expandedNodes.current is mutated and
where setGraphData's merge logic runs so stale subgraphs are never injected.
- Around line 34-64: Replace the manual useEffect/useState fetch with a React
Query query: remove the effect that calls graphApi.getEntityGraph and the
cancelled flag, and instead create a useQuery keyed by [ "entityGraph",
projectId, focusIri, branch, showDescendants, resetKey ] that calls
graphApi.getEntityGraph(projectId, focusIri, { branch, ancestorsDepth: 5,
descendantsDepth: showDescendants ? 2 : 0 }); map the query's
data/isLoading/error to the existing state handlers (setGraphData, setIsLoading)
or eliminate local state in favor of query data/isLoading, and keep
expandedNodes.current initialization when focusIri changes (e.g., in a
lightweight effect) so existing behavior around expandedNodes.current = new
Set([focusIri]) remains.
🪄 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
Run ID: 035be297-2d2f-4ff5-8d12-1ffa801ceb30
📒 Files selected for processing (5)
__tests__/lib/api/graph.test.ts__tests__/lib/hooks/useGraphData.test.tscomponents/graph/EntityGraphModal.tsxlib/api/graph.tslib/hooks/useGraphData.ts
✅ Files skipped from review due to trivial changes (1)
- tests/lib/api/graph.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- components/graph/EntityGraphModal.tsx
- tests/lib/hooks/useGraphData.test.ts
- lib/api/graph.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
lib/graph/useELKLayout.ts (1)
78-81:⚠️ Potential issue | 🟠 MajorNormalize
node_typeinstead of force-casting it.
(backendNode.node_type || "class") as GraphNodeTypebypasses validation and can silently misclassify backend values (notablysecondary_root), which can break node/MiniMap semantics.Suggested fix
+const normalizeNodeType = (nodeType?: string | null): GraphNodeType => { + switch (nodeType) { + case "class": + case "focus": + case "ancestor": + case "root": + case "secondary_root": + return nodeType; + default: + return "class"; + } +}; ... - nodeType: (backendNode.node_type || "class") as GraphNodeType, + nodeType: normalizeNodeType(backendNode.node_type),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/graph/useELKLayout.ts` around lines 78 - 81, The code is force-casting backendNode.node_type to GraphNodeType which skips validation and can misclassify values like "secondary_root"; update the mapping in useELKLayout.ts where data.nodeType is set (currently "(backendNode.node_type || 'class') as GraphNodeType") to call a normalization helper (e.g., normalizeNodeType) or inline switch that maps known backend strings to the correct GraphNodeType (handle "secondary_root" explicitly, map unknown/undefined to "class"), and use that normalized value for data.nodeType so types are validated at runtime instead of using a blind cast.lib/hooks/useGraphData.ts (1)
80-103:⚠️ Potential issue | 🟠 MajorScope expansion responses to the active graph.
An expansion started for graph A can still merge into graph B if
focusIrichanges orresetGraph()runs before the promise resolves. The updater currently applies into whateverprevexists at settle time, so stale responses can repopulate or corrupt the new graph. Capture a graph version/request key when dispatching the request and ignore results that no longer match the active graph.
🧹 Nitpick comments (2)
__tests__/components/graph/OntologyEdge.test.tsx (2)
50-60: These two tests are currently too weak for what their names claim.Line 50 and Line 159 describe
subClassOfdefault behavior, but assertions only check element existence (plus a duplicatetoBeDefined()at Line 59). Consider either asserting default edge semantics or renaming the tests to match current checks.Also applies to: 159-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/graph/OntologyEdge.test.tsx` around lines 50 - 60, The two tests claiming "renders the base edge with subClassOf style by default" are weak and contain a duplicate expect; update the tests for the OntologyEdge component to either assert actual default semantics (e.g., check className, data-edge-type, stroke/dash attributes, or other indicators of subClassOf style on the element with testId "base-edge-edge-1" or on related DOM nodes generated from <OntologyEdge {...baseProps} />) or rename the test descriptions to reflect that they only assert presence; also remove the duplicated expect(edge).toBeDefined() and ensure you reference the same unique identifiers (OntologyEdge, baseProps, testId "base-edge-edge-1") when adding the stronger assertions or renaming.
5-8: Add one test case forgraphEdgeStyle: "bezier"path selection.Right now this suite hardcodes
"smoothstep", so the bezier branch introduced by the new toggle is not validated here.Also applies to: 24-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/graph/OntologyEdge.test.tsx` around lines 5 - 8, The test suite currently hardcodes useEditorModeStore to return graphEdgeStyle: "smoothstep", so the newly added "bezier" branch isn't covered; add a new test (or duplicate the existing one) in __tests__/components/graph/OntologyEdge.test.tsx that mocks useEditorModeStore to return graphEdgeStyle: "bezier" and then render the same component/assertions to validate the bezier path selection, referencing the useEditorModeStore mock and the graphEdgeStyle selector to drive the alternate branch for OntologyEdge (or whichever component renders the path).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/hooks/useGraphData.ts`:
- Around line 73-79: The descendants toggle is ignored because getEntityGraph
calls (in useGraphData.ts) and the expandNode() helper always request one
descendant hop; change those calls to pass descendantsDepth based on the "show
descendants" state (e.g., descendantsDepth: showDescendants ? 1 : 0) and update
expandNode() so it computes and forwards descendantsDepth from the current
toolbar/toggle state instead of hardcoding 1; ensure both places that call
graphApi.getEntityGraph (the initial load and the expandNode invocation) use
this conditional descendantsDepth so descendants only appear when the toggle is
enabled.
- Around line 47-66: The effect in useGraphData calls
graphApi.getEntityGraph(projectId, focusIri, ..., accessToken) but omits
accessToken from the dependency array, so update the effect's deps to include
accessToken; specifically modify the useEffect that calls
graphApi.getEntityGraph (the callback that sets setGraphData and setIsLoading
and uses the cancelled flag) and add accessToken to the dependency list so the
fetch retries when the token becomes available or changes.
---
Duplicate comments:
In `@lib/graph/useELKLayout.ts`:
- Around line 78-81: The code is force-casting backendNode.node_type to
GraphNodeType which skips validation and can misclassify values like
"secondary_root"; update the mapping in useELKLayout.ts where data.nodeType is
set (currently "(backendNode.node_type || 'class') as GraphNodeType") to call a
normalization helper (e.g., normalizeNodeType) or inline switch that maps known
backend strings to the correct GraphNodeType (handle "secondary_root"
explicitly, map unknown/undefined to "class"), and use that normalized value for
data.nodeType so types are validated at runtime instead of using a blind cast.
---
Nitpick comments:
In `@__tests__/components/graph/OntologyEdge.test.tsx`:
- Around line 50-60: The two tests claiming "renders the base edge with
subClassOf style by default" are weak and contain a duplicate expect; update the
tests for the OntologyEdge component to either assert actual default semantics
(e.g., check className, data-edge-type, stroke/dash attributes, or other
indicators of subClassOf style on the element with testId "base-edge-edge-1" or
on related DOM nodes generated from <OntologyEdge {...baseProps} />) or rename
the test descriptions to reflect that they only assert presence; also remove the
duplicated expect(edge).toBeDefined() and ensure you reference the same unique
identifiers (OntologyEdge, baseProps, testId "base-edge-edge-1") when adding the
stronger assertions or renaming.
- Around line 5-8: The test suite currently hardcodes useEditorModeStore to
return graphEdgeStyle: "smoothstep", so the newly added "bezier" branch isn't
covered; add a new test (or duplicate the existing one) in
__tests__/components/graph/OntologyEdge.test.tsx that mocks useEditorModeStore
to return graphEdgeStyle: "bezier" and then render the same component/assertions
to validate the bezier path selection, referencing the useEditorModeStore mock
and the graphEdgeStyle selector to drive the alternate branch for OntologyEdge
(or whichever component renders the path).
🪄 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
Run ID: b722a5cc-847d-48e4-9bda-321d8e1178d6
📒 Files selected for processing (15)
__tests__/components/graph/OntologyEdge.test.tsx__tests__/components/graph/OntologyGraph.test.tsx__tests__/lib/api/graph.test.ts__tests__/lib/graph/useELKLayout.test.ts__tests__/lib/hooks/useGraphData.test.tscomponents/editor/developer/DeveloperEditorLayout.tsxcomponents/editor/standard/StandardEditorLayout.tsxcomponents/graph/EntityGraphModal.tsxcomponents/graph/OntologyEdge.tsxcomponents/graph/OntologyGraph.tsxcomponents/graph/OntologyNode.tsxlib/api/graph.tslib/graph/useELKLayout.tslib/hooks/useGraphData.tslib/stores/editorModeStore.ts
✅ Files skipped from review due to trivial changes (2)
- tests/lib/api/graph.test.ts
- components/graph/EntityGraphModal.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- components/graph/OntologyNode.tsx
- components/graph/OntologyEdge.tsx
- tests/components/graph/OntologyGraph.test.tsx
- lib/api/graph.ts
- components/graph/OntologyGraph.tsx
- tests/lib/graph/useELKLayout.test.ts
- tests/lib/hooks/useGraphData.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
lib/hooks/useGraphData.ts (2)
73-103:⚠️ Potential issue | 🟠 MajorDrop expansion responses that outlive the current graph.
This request is not scoped to the
focusIri/reset state that started it. If the user changes focus, toggles descendants, or callsresetGraph()before it resolves, the.then()block can still mergenewDatainto the new graph viaif (!prev) return newData, and it will also mark that stale node as expanded. Guard expansions with a graph/version token and anexpandingNodesset so stale or duplicate requests are ignored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useGraphData.ts` around lines 73 - 103, The expansion promise from graphApi.getEntityGraph must be guarded so its .then() does not merge stale results after focus changes or reset; add a graph version/token (updated when focusIri, showDescendants or resetGraph() changes) and an expandingNodes Set to track active expansions, attach the token to the request, and inside the .then() first verify the token matches the current graph token and that the iri is still in expandingNodes (and not already expanded via expandedNodes.current) before calling setGraphData or expandedNodes.current.add(iri); if the token mismatches or the iri is absent, drop the response and do not modify graph state.
29-66: 🛠️ Refactor suggestion | 🟠 MajorMove this server-state logic onto React Query.
Both the baseline load and the expansion path are still hand-rolled request lifecycles. That keeps caching, deduplication, retries, and invalidation logic inside the hook instead of using the repo-standard
useQuery/useMutationflow.As per coding guidelines, "Use React Query (
@tanstack/react-query) for server state management."Also applies to: 69-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useGraphData.ts` around lines 29 - 66, Replace the manual fetch lifecycle in useGraphData with React Query: remove local isLoading/graphData state and use useQuery to call graphApi.getEntityGraph from inside the useGraphData hook (e.g. const query = useQuery(['entityGraph', projectId, focusIri, branch, showDescendants, resetKey], () => graphApi.getEntityGraph(projectId, focusIri, { branch, ancestorsDepth: 5, descendantsDepth: showDescendants ? 2 : 0 }, accessToken), { enabled: !!focusIri });), return query.data/query.isLoading instead of graphData/isLoading, and keep expandedNodes.current = new Set([focusIri]) in an effect when focusIri changes; remove manual cancelled/then/catch/finally logic because React Query handles cancellation, retries and caching; ensure cache key includes all inputs (projectId, focusIri, branch, showDescendants, resetKey, accessToken if needed) so invalidation/deduplication works as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/hooks/useGraphData.ts`:
- Around line 73-103: The expansion promise from graphApi.getEntityGraph must be
guarded so its .then() does not merge stale results after focus changes or
reset; add a graph version/token (updated when focusIri, showDescendants or
resetGraph() changes) and an expandingNodes Set to track active expansions,
attach the token to the request, and inside the .then() first verify the token
matches the current graph token and that the iri is still in expandingNodes (and
not already expanded via expandedNodes.current) before calling setGraphData or
expandedNodes.current.add(iri); if the token mismatches or the iri is absent,
drop the response and do not modify graph state.
- Around line 29-66: Replace the manual fetch lifecycle in useGraphData with
React Query: remove local isLoading/graphData state and use useQuery to call
graphApi.getEntityGraph from inside the useGraphData hook (e.g. const query =
useQuery(['entityGraph', projectId, focusIri, branch, showDescendants,
resetKey], () => graphApi.getEntityGraph(projectId, focusIri, { branch,
ancestorsDepth: 5, descendantsDepth: showDescendants ? 2 : 0 }, accessToken), {
enabled: !!focusIri });), return query.data/query.isLoading instead of
graphData/isLoading, and keep expandedNodes.current = new Set([focusIri]) in an
effect when focusIri changes; remove manual cancelled/then/catch/finally logic
because React Query handles cancellation, retries and caching; ensure cache key
includes all inputs (projectId, focusIri, branch, showDescendants, resetKey,
accessToken if needed) so invalidation/deduplication works as intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f05d3a6-7a9c-42f3-8341-563c6feaddcb
📒 Files selected for processing (3)
__tests__/components/graph/OntologyEdge.test.tsxlib/graph/useELKLayout.tslib/hooks/useGraphData.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/components/graph/OntologyEdge.test.tsx
- lib/graph/useELKLayout.ts
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 `@lib/hooks/useGraphData.ts`:
- Around line 76-87: The effect currently resets expansion state when query.data
changes, causing unwanted resets on background refetches; update the useEffect
that manipulates graphEpoch, expandedNodes, expandingNodes, and setExpansions so
it depends only on the actual graph input props (projectId, focusIri, branch,
showDescendants, resetKey) instead of query.data, ensuring expansions are
cleared only when the graph context truly changes.
- Around line 67-77: The queryKey currently uses !!accessToken which only tracks
boolean auth state; change the key to include the actual access token (or a
stable token hash/ID) so TanStack Query cache distinguishes different
tokens—update the queryKey in useGraphData (the useQuery call) to include
accessToken (or a hashed/tokenId) instead of !!accessToken, and ensure
graphApi.getEntityGraph still receives accessToken via the existing queryFn;
alternatively, move token resolution into queryFn so the key does not rely on
boolean auth state and cache coherency is preserved (references: useQuery,
queryKey, queryFn, graphApi.getEntityGraph, accessToken).
🪄 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
Run ID: 47a2e120-3520-422e-af49-187b3eafd7f1
📒 Files selected for processing (2)
__tests__/lib/hooks/useGraphData.test.tslib/hooks/useGraphData.ts
Member
Graph view interaction design — needs clarificationThe current click/interaction behavior needs refinement before merging. Here's what happens now and what should be discussed: Current behavior
Issues observed
Proposed interaction modelConsider adding clickable elements within each node to separate concerns:
This would mirror the class tree's interaction model (click to select, caret to expand) and be immediately intuitive. Viewport followRegardless of which interaction model we choose, the viewport should smoothly transition to keep the acted-upon node visible after layout changes. React Flow's @damienriehl — what's your preferred interaction model here? |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
components/graph/OntologyGraph.tsx (2)
173-185:⚠️ Potential issue | 🟠 MajorClear React Flow state when the source graph disappears.
If
graphDatabecomesnullor empty, this path stops running layout but never clears the previousnodes/edges, so Reset, empty responses, and fetch failures can keep rendering the last graph.Minimal fix
useEffect(() => { + if (!graphData || graphData.nodes.length === 0) { + setNodes([]); + setEdges([]); + return; + } setNodes(layoutNodes); setEdges(layoutEdges); - }, [layoutNodes, layoutEdges, setNodes, setEdges]); + }, [graphData, layoutNodes, layoutEdges, setNodes, setEdges]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/graph/OntologyGraph.tsx` around lines 173 - 185, When graphData becomes null or has no nodes the current effect returns early but leaves the previous React Flow state intact; modify the first useEffect (the one that calls runLayout) to explicitly clear React Flow state by calling setNodes([]) and setEdges([]) when graphData is falsy or graphData.nodes.length === 0 instead of just returning, so the UI no longer shows the stale graph; keep the existing runLayout call and error handling for the non-empty case.
187-203:⚠️ Potential issue | 🟠 MajorDon’t duplicate expansion state in the component.
useGraphData.expandNode()already dedupes expanded and in-flight requests. This extraexpandedNodesref gets out of sync: it only resets onfocusIri, so Reset/show-descendants rebuilds leave stale ids here, and Line 198 marks a node expanded before the async fetch succeeds, which makes failed expansions permanently unclickable. Let the hook own this state.Lean fix
- // Progressive expansion: track which nodes have been clicked to expand - const expandedNodes = useRef(new Set<string>(focusIri ? [focusIri] : [])); - - // Reset expanded set when focus changes - useEffect(() => { - expandedNodes.current = new Set<string>(focusIri ? [focusIri] : []); - }, [focusIri]); - const handleNodeClick: NodeMouseHandler = useCallback( (_event, node) => { - if (!expandedNodes.current.has(node.id)) { - expandedNodes.current.add(node.id); - expandNode(node.id); - } + expandNode(node.id); }, [expandNode], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/graph/OntologyGraph.tsx` around lines 187 - 203, The component maintains a duplicate expandedNodes ref and effect (expandedNodes and its useEffect) which conflicts with useGraphData.expandNode's internal deduping and in-flight tracking; remove the expandedNodes ref and its resetting useEffect entirely and simplify handleNodeClick to just call expandNode(node.id) (no local has-check), ensuring handleNodeClick depends only on expandNode so the hook fully owns expansion state and prevents stale/unremovable IDs.lib/hooks/useGraphData.ts (1)
67-74:⚠️ Potential issue | 🟠 MajorThe query key still collapses every authenticated session into one cache entry.
!!accessTokenonly distinguishes “token present” vs “missing”. If the token refreshes, or the signed-in user changes while both values are truthy, React Query keeps reusing the same cached graph for Line 68 instead of treating it as a fresh auth context. Either key this query by a stable auth discriminator, or explicitly invalidate it when auth changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useGraphData.ts` around lines 67 - 74, The queryKey in useQuery (in useGraphData.ts) currently uses !!accessToken which only distinguishes presence vs absence and collapses different authenticated sessions; update the query key to include a stable auth discriminator (for example include the raw accessToken or a safer stable id like auth.user.id or auth.sessionId/tokenVersion) instead of !!accessToken, or add logic to explicitly invalidate the ["entityGraph", projectId, focusIri, branch, showAllDescendants, resetKey] query when the auth context changes; locate useQuery and graphApi.getEntityGraph to implement this change so each distinct authentication context yields a separate cache entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/hooks/useGraphData.ts`:
- Around line 79-85: The reset of graphEpoch.current and expansion state must
run synchronously in the commit phase to prevent stale expandNode() promise
callbacks from passing the epoch guard; replace the useEffect block that updates
graphEpoch.current, expandedNodes.current, expandingNodes.current, and
setExpansions([]) with useLayoutEffect so the reset happens before promise
microtasks run, and ensure useLayoutEffect is imported from React where
useGraphData.ts declares the hook.
---
Duplicate comments:
In `@components/graph/OntologyGraph.tsx`:
- Around line 173-185: When graphData becomes null or has no nodes the current
effect returns early but leaves the previous React Flow state intact; modify the
first useEffect (the one that calls runLayout) to explicitly clear React Flow
state by calling setNodes([]) and setEdges([]) when graphData is falsy or
graphData.nodes.length === 0 instead of just returning, so the UI no longer
shows the stale graph; keep the existing runLayout call and error handling for
the non-empty case.
- Around line 187-203: The component maintains a duplicate expandedNodes ref and
effect (expandedNodes and its useEffect) which conflicts with
useGraphData.expandNode's internal deduping and in-flight tracking; remove the
expandedNodes ref and its resetting useEffect entirely and simplify
handleNodeClick to just call expandNode(node.id) (no local has-check), ensuring
handleNodeClick depends only on expandNode so the hook fully owns expansion
state and prevents stale/unremovable IDs.
In `@lib/hooks/useGraphData.ts`:
- Around line 67-74: The queryKey in useQuery (in useGraphData.ts) currently
uses !!accessToken which only distinguishes presence vs absence and collapses
different authenticated sessions; update the query key to include a stable auth
discriminator (for example include the raw accessToken or a safer stable id like
auth.user.id or auth.sessionId/tokenVersion) instead of !!accessToken, or add
logic to explicitly invalidate the ["entityGraph", projectId, focusIri, branch,
showAllDescendants, resetKey] query when the auth context changes; locate
useQuery and graphApi.getEntityGraph to implement this change so each distinct
authentication context yields a separate cache entry.
🪄 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
Run ID: e5ca5302-428e-40e4-954f-7648ab8a00f2
📒 Files selected for processing (4)
__tests__/components/graph/OntologyGraph.test.tsx__tests__/lib/hooks/useGraphData.test.tscomponents/graph/OntologyGraph.tsxlib/hooks/useGraphData.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
__tests__/components/graph/OntologyGraph.test.tsx (2)
280-283: Add an explicit test forisLayouting: trueUI.The new “Computing layout...” branch is behaviorally important but not directly asserted. Add one test that mocks
useELKLayout().isLayouting = trueand checks the indicator is rendered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/graph/OntologyGraph.test.tsx` around lines 280 - 283, Add a new unit test in OntologyGraph.test.tsx that explicitly asserts the "Computing layout..." UI when useELKLayout reports isLayouting: true: mock the useELKLayout hook (the same mock used elsewhere in the test file) to return { isLayouting: true, ... } for that test case, render the OntologyGraph component (or the render helper already used), and expect the "Computing layout..." indicator to be in the document; restore/reset the mock after the test to avoid affecting other tests.
234-243: AssertaccessTokenis forwarded touseGraphData.This test currently verifies
focusIri/projectId/branchonly. Please includeaccessTokenin props and assert it is passed through, to lock auth propagation behavior.✅ Tighten this test
const defaultProps = { focusIri: "iri:Class1", projectId: "proj-1", branch: "main", + accessToken: "token-123", onNavigateToClass: vi.fn(), }; @@ expect.objectContaining({ focusIri: "iri:Class1", projectId: "proj-1", branch: "main", + accessToken: "token-123", })As per coding guidelines "Access tokens must be passed to API calls via
session.accessToken."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/graph/OntologyGraph.test.tsx` around lines 234 - 243, The test should assert that the access token is forwarded to useGraphData: update the rendered props so defaultProps includes a session.accessToken (or ensure defaultProps.session.accessToken is set) when calling render(<OntologyGraph {...defaultProps} />), and extend the expect(mockUseGraphData).toHaveBeenCalledWith(...) objectContaining to include accessToken: defaultProps.session.accessToken (or the literal token string used in defaultProps); ensure you reference the OntologyGraph render and mockUseGraphData/useGraphData call sites when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/graph/OntologyGraph.tsx`:
- Around line 180-182: The runLayout(graphData, direction) call currently
swallows errors; update the catch handler in OntologyGraph.tsx to surface
failures by logging the error (including the error object and contextual info
like graphData.id or direction) and/or setting a local React error state (e.g.,
layoutError via useState) so the UI or monitoring can show the failure; keep the
existing behavior of not throwing, but ensure processLogger.error/console.error
is called with a descriptive message and the caught error inside the catch of
runLayout.
- Around line 191-203: The click handler currently calls expandNode immediately
which runs before the double-click navigation; change handleNodeClick and
handleNodeDoubleClick to use a short click-delay guard: in handleNodeClick start
a timeout (e.g., ~200ms) stored in a ref and call expandNode only when the
timeout fires, and in handleNodeDoubleClick clear that timeout then immediately
call onNavigateToClass; ensure the timeout ref is cleared on unmount (or on
subsequent clicks) to avoid leaks and update hook dependencies to include the
ref/cleanup logic while keeping the unique symbols handleNodeClick,
handleNodeDoubleClick, expandNode, and onNavigateToClass.
---
Nitpick comments:
In `@__tests__/components/graph/OntologyGraph.test.tsx`:
- Around line 280-283: Add a new unit test in OntologyGraph.test.tsx that
explicitly asserts the "Computing layout..." UI when useELKLayout reports
isLayouting: true: mock the useELKLayout hook (the same mock used elsewhere in
the test file) to return { isLayouting: true, ... } for that test case, render
the OntologyGraph component (or the render helper already used), and expect the
"Computing layout..." indicator to be in the document; restore/reset the mock
after the test to avoid affecting other tests.
- Around line 234-243: The test should assert that the access token is forwarded
to useGraphData: update the rendered props so defaultProps includes a
session.accessToken (or ensure defaultProps.session.accessToken is set) when
calling render(<OntologyGraph {...defaultProps} />), and extend the
expect(mockUseGraphData).toHaveBeenCalledWith(...) objectContaining to include
accessToken: defaultProps.session.accessToken (or the literal token string used
in defaultProps); ensure you reference the OntologyGraph render and
mockUseGraphData/useGraphData call sites when making this change.
🪄 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
Run ID: 64378071-71db-48fc-b87e-f250c7f970e0
📒 Files selected for processing (3)
__tests__/components/graph/OntologyGraph.test.tsxcomponents/graph/OntologyGraph.tsxlib/hooks/useGraphData.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/hooks/useGraphData.ts
JohnRDOrazio
force-pushed
the
entity-graph-pr
branch
from
76c924d to
9392032
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
.planning/features/entity-graph-port/CONTEXT.md (1)
23-26:⚠️ Potential issue | 🟡 MinorUpdate the documented graph endpoint to match the client contract.
D-05still describes a path parameter form plusdescendants_depth=2by default, but the shipped client usesGET /api/v1/projects/{projectId}/ontology/classes/graphwithclass_iriin query params, and the initial fetch usesdescendants_depth=1. Leaving this as-is will send follow-up planning and manual testing toward the wrong API shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/features/entity-graph-port/CONTEXT.md around lines 23 - 26, Update the documented graph endpoint D-05 to match the client contract: change the path to GET /api/v1/projects/{projectId}/ontology/classes/graph, move class_iri from a path parameter into query parameters, and set the initial/default descendants_depth to 1 (not 2); ensure the example client call (graphApi.getEntityGraph) and the described query params (ancestors_depth, descendants_depth, max_nodes, include_see_also) reflect this shape and still note the returned EntityGraphResponse (nodes, edges, truncated flag, total concept count).components/graph/OntologyNode.tsx (1)
93-131:⚠️ Potential issue | 🟠 MajorLR layout still needs directional handles.
The source/target swap fixes top-down flow, but the handles are still hardcoded to
Top/Bottom. When the graph switches to left-right layout, edges will keep anchoring vertically instead of fromLeft/Right, so routing will still look wrong there.♻️ Suggested direction-aware change
export interface OntologyNodeData { [key: string]: unknown; label: string; nodeType: GraphNodeType; + layoutDirection?: "TB" | "LR"; deprecated?: boolean; childCount?: number; isExpanded?: boolean; onNavigate?: (iri: string) => void; onExpandNode?: (iri: string) => void; } @@ export const OntologyNode = memo(function OntologyNode({ data, id, }: OntologyNodeProps) { const { label, nodeType, deprecated, childCount, isExpanded, onNavigate, onExpandNode } = data; + const isLeftRight = data.layoutDirection === "LR"; @@ - <Handle type="target" position={Position.Top} className="bg-slate-400! w-2! h-2! border-0!" /> + <Handle + type="target" + position={isLeftRight ? Position.Left : Position.Top} + className="bg-slate-400! w-2! h-2! border-0!" + /> @@ - <Handle type="source" position={Position.Bottom} className="bg-slate-400! w-2! h-2! border-0!" /> + <Handle + type="source" + position={isLeftRight ? Position.Right : Position.Bottom} + className="bg-slate-400! w-2! h-2! border-0!" + />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/graph/OntologyNode.tsx` around lines 93 - 131, The handles are hardcoded to Position.Top/Bottom which breaks left-right layouts; update the Handle components in OntologyNode to pick their position from the current graph layout (e.g., use a layout prop or a layout selector) and map the layout to Position.Top/Bottom for top-down or Position.Left/Right for left-right flow; change both the target Handle and source Handle to use that computed position (reference the Handle components and Position enum) so edges anchor correctly when layout switches.
🧹 Nitpick comments (3)
components/editor/standard/StandardEditorLayout.tsx (1)
17-18: KeepEntityGraphModallazy-loaded.This static import makes the modal part of the main editor bundle even though it is only reachable behind the inline graph’s “Expand” action. That loses the lazy-load behavior called out for this feature and makes the editor pay the graph-modal cost up front.
📦 Suggested import pattern
-import { EntityGraphModal } from "@/components/graph/EntityGraphModal"; +const EntityGraphModal = dynamic( + () => import("@/components/graph/EntityGraphModal").then((mod) => mod.EntityGraphModal), + { ssr: false }, +);Also applies to: 456-470
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/editor/standard/StandardEditorLayout.tsx` around lines 17 - 18, The static import of EntityGraphModal in StandardEditorLayout forces the modal into the main bundle; change it to a lazy/dynamic import so the modal is only loaded when needed (e.g., use React.lazy + Suspense or Next.js dynamic import) and replace direct usage of EntityGraphModal in the component with the lazy-loaded component (referencing EntityGraphModal usage in StandardEditorLayout and the related code around lines 456-470). Ensure you include a fallback UI while loading and preserve any props/API shape so callers in StandardEditorLayout remain unchanged.components/graph/OntologyGraph.tsx (1)
194-214: Consider centering viewport on the expanded node after layout.Per the PR discussion, after clicking a node to expand, the acted-upon node can jump out of view once ELK relayout completes. The current
fitViewfits all nodes but doesn't prioritize the clicked node.💡 Suggested approach using React Flow's setCenter
+import { useReactFlow } from "@xyflow/react"; // Inside the component: +const { setCenter, getNode } = useReactFlow(); const handleNodeClick: NodeMouseHandler = useCallback( (_event, node) => { if (clickTimerRef.current) clearTimeout(clickTimerRef.current); clickTimerRef.current = setTimeout(() => { clickTimerRef.current = null; + // Store node position before expansion for re-centering after layout + const clickedNode = getNode(node.id); expandNode(node.id); + // After layout completes, center on this node (could be done via effect) }, 200); }, - [expandNode], + [expandNode, getNode], );Alternatively, track the last-expanded node ID and use a
useEffectto callsetCenterafterlayoutNodesupdates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/graph/OntologyGraph.tsx` around lines 194 - 214, After expanding a node the ELK relayout can move the clicked node out of view; modify the click/expand flow to center the viewport on the expanded node after layout completes: record the expanded node id (e.g., set a lastExpandedId in handleNodeClick/expandNode path using clickTimerRef and expandNode), then add a useEffect that watches layoutNodes (or the variable updated when ELK finishes) and when it changes and lastExpandedId is set, call the React Flow setCenter (or fitView with center) to center on lastExpandedId, then clear lastExpandedId; reference handleNodeClick, expandNode, clickTimerRef, layoutNodes and setCenter/useEffect when making the change.lib/graph/useELKLayout.ts (1)
100-111: Consider validatingedge_typebefore casting.The
edge_type as GraphEdgeTypecast assumes the backend always sends a valid type. If the backend introduces a new edge type or sends an unexpected value,OntologyEdgewill receive an unrecognizededgeTypeand may fail to find matching styles inedgeTypeConfig.🛡️ Defensive validation approach
+const VALID_EDGE_TYPES = new Set<GraphEdgeType>([ + "subClassOf", "equivalentClass", "disjointWith", "seeAlso", +]); + +function normalizeEdgeType(raw: string | undefined): GraphEdgeType { + if (raw && VALID_EDGE_TYPES.has(raw as GraphEdgeType)) return raw as GraphEdgeType; + return "subClassOf"; // fallback to most common type +} const layoutedEdges: Edge<OntologyEdgeData>[] = data.edges.map((e) => ({ id: e.id, source: e.source, target: e.target, type: "ontologyEdge", ...(e.edge_type === "subClassOf" && { markerEnd: { type: MarkerType.ArrowClosed, color: "#94a3b8", width: 16, height: 16 }, }), data: { - edgeType: e.edge_type as GraphEdgeType, + edgeType: normalizeEdgeType(e.edge_type), }, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/graph/useELKLayout.ts` around lines 100 - 111, Validate e.edge_type before casting to GraphEdgeType when building layoutedEdges: check the incoming e.edge_type against the known GraphEdgeType members or keys of edgeTypeConfig inside the data.edges.map callback and only cast when it matches; if it does not match, set a safe fallback value (e.g., a default GraphEdgeType or 'unknown') and optionally log or warn so OntologyEdge and edge styling lookup won't receive an invalid enum value. Ensure you update the mapping where data: { edgeType: ... } is assigned and use the same whitelist logic if other places reference GraphEdgeType.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/lib/graph/useELKLayout.test.ts`:
- Around line 185-196: The test never observes the loading state because it
awaits runLayout() to completion; change the test to mock the ELK/layout call
used by useELKLayout so it returns a deferred promise, call
result.current.runLayout(data) without awaiting it, assert
result.current.isLayouting === true while the deferred promise is still pending,
then resolve the deferred promise and await completion and finally assert
isLayouting === false; target the useELKLayout hook and its runLayout method,
the isLayouting flag, and reuse makeGraphResponse/renderHook/act to orchestrate
the assertions.
In @.planning/features/entity-graph-port/CONTEXT.md:
- Around line 73-83: Replace the absolute workstation paths in
.planning/features/entity-graph-port/CONTEXT.md with repository-relative paths
or stable refs (commit SHAs or GitHub URLs) so other contributors can open the
listed files; for example, change
`/home/.../packages/ui/src/components/mapping/graph/EntityGraph.tsx` etc. to
paths like `packages/ui/src/components/mapping/graph/EntityGraph.tsx` or to full
repo URLs/commit SHAs for each referenced file (EntityGraph.tsx,
ConceptNode.tsx, HierarchyEdge.tsx, useELKLayout.ts, EntityGraphModal.tsx,
graph-types.ts, api-client.ts, graph_models.py, folio_service.py, mapping.py)
and verify each link/reference resolves in the repo.
In `@lib/hooks/useGraphData.ts`:
- Around line 67-77: The query currently can start when accessToken is undefined
causing unauthorized requests; update the useQuery enabled condition to require
a valid access token (e.g. enabled: !!focusIri && !!accessToken) so the query
(query variable calling graphApi.getEntityGraph) only runs after
session.accessToken is available; keep accessToken in the queryKey and pass
session.accessToken into graphApi.getEntityGraph as before.
---
Duplicate comments:
In @.planning/features/entity-graph-port/CONTEXT.md:
- Around line 23-26: Update the documented graph endpoint D-05 to match the
client contract: change the path to GET
/api/v1/projects/{projectId}/ontology/classes/graph, move class_iri from a path
parameter into query parameters, and set the initial/default descendants_depth
to 1 (not 2); ensure the example client call (graphApi.getEntityGraph) and the
described query params (ancestors_depth, descendants_depth, max_nodes,
include_see_also) reflect this shape and still note the returned
EntityGraphResponse (nodes, edges, truncated flag, total concept count).
In `@components/graph/OntologyNode.tsx`:
- Around line 93-131: The handles are hardcoded to Position.Top/Bottom which
breaks left-right layouts; update the Handle components in OntologyNode to pick
their position from the current graph layout (e.g., use a layout prop or a
layout selector) and map the layout to Position.Top/Bottom for top-down or
Position.Left/Right for left-right flow; change both the target Handle and
source Handle to use that computed position (reference the Handle components and
Position enum) so edges anchor correctly when layout switches.
---
Nitpick comments:
In `@components/editor/standard/StandardEditorLayout.tsx`:
- Around line 17-18: The static import of EntityGraphModal in
StandardEditorLayout forces the modal into the main bundle; change it to a
lazy/dynamic import so the modal is only loaded when needed (e.g., use
React.lazy + Suspense or Next.js dynamic import) and replace direct usage of
EntityGraphModal in the component with the lazy-loaded component (referencing
EntityGraphModal usage in StandardEditorLayout and the related code around lines
456-470). Ensure you include a fallback UI while loading and preserve any
props/API shape so callers in StandardEditorLayout remain unchanged.
In `@components/graph/OntologyGraph.tsx`:
- Around line 194-214: After expanding a node the ELK relayout can move the
clicked node out of view; modify the click/expand flow to center the viewport on
the expanded node after layout completes: record the expanded node id (e.g., set
a lastExpandedId in handleNodeClick/expandNode path using clickTimerRef and
expandNode), then add a useEffect that watches layoutNodes (or the variable
updated when ELK finishes) and when it changes and lastExpandedId is set, call
the React Flow setCenter (or fitView with center) to center on lastExpandedId,
then clear lastExpandedId; reference handleNodeClick, expandNode, clickTimerRef,
layoutNodes and setCenter/useEffect when making the change.
In `@lib/graph/useELKLayout.ts`:
- Around line 100-111: Validate e.edge_type before casting to GraphEdgeType when
building layoutedEdges: check the incoming e.edge_type against the known
GraphEdgeType members or keys of edgeTypeConfig inside the data.edges.map
callback and only cast when it matches; if it does not match, set a safe
fallback value (e.g., a default GraphEdgeType or 'unknown') and optionally log
or warn so OntologyEdge and edge styling lookup won't receive an invalid enum
value. Ensure you update the mapping where data: { edgeType: ... } is assigned
and use the same whitelist logic if other places reference GraphEdgeType.
🪄 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
Run ID: 7b9be626-c09f-4899-acaf-ba645ab47af7
📒 Files selected for processing (25)
.planning/features/entity-graph-port/CONTEXT.md.planning/features/entity-graph-port/HANDOFF.md.planning/features/entity-graph-port/PLAN.md__tests__/components/graph/EntityGraphModal.test.tsx__tests__/components/graph/OntologyEdge.test.tsx__tests__/components/graph/OntologyGraph.test.tsx__tests__/lib/api/graph.test.ts__tests__/lib/graph/buildGraphData.test.ts__tests__/lib/graph/elkLayout.test.ts__tests__/lib/graph/useELKLayout.test.ts__tests__/lib/graph/utils.test.ts__tests__/lib/hooks/useGraphData.test.tscomponents/editor/developer/DeveloperEditorLayout.tsxcomponents/editor/standard/StandardEditorLayout.tsxcomponents/graph/EntityGraphModal.tsxcomponents/graph/OntologyEdge.tsxcomponents/graph/OntologyGraph.tsxcomponents/graph/OntologyNode.tsxlib/api/graph.tslib/graph/buildGraphData.tslib/graph/elkLayout.tslib/graph/useELKLayout.tslib/graph/utils.tslib/hooks/useGraphData.tslib/stores/editorModeStore.ts
💤 Files with no reviewable changes (4)
- lib/graph/elkLayout.ts
- tests/lib/graph/elkLayout.test.ts
- lib/graph/buildGraphData.ts
- tests/lib/graph/buildGraphData.test.ts
✅ Files skipped from review due to trivial changes (7)
- lib/graph/utils.ts
- tests/lib/graph/utils.test.ts
- tests/lib/api/graph.test.ts
- .planning/features/entity-graph-port/HANDOFF.md
- .planning/features/entity-graph-port/PLAN.md
- components/editor/developer/DeveloperEditorLayout.tsx
- lib/api/graph.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/stores/editorModeStore.ts
- tests/components/graph/EntityGraphModal.test.tsx
- components/graph/EntityGraphModal.tsx
- components/graph/OntologyEdge.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (4)
components/graph/OntologyGraph.tsx (1)
202-213: Viewport centering uses magic offset values.The
setCentercall uses hardcoded offsets (x + 90,y + 22) that appear to be half the node dimensions. Consider deriving these fromNODE_WIDTH/NODE_HEIGHTconstants or computing from actual node dimensions for maintainability.💡 Suggested improvement
// If node dimensions are known (e.g., 180x44 from useELKLayout): const NODE_CENTER_X_OFFSET = 90; // NODE_WIDTH / 2 const NODE_CENTER_Y_OFFSET = 22; // NODE_HEIGHT / 2 setCenter( target.position.x + NODE_CENTER_X_OFFSET, target.position.y + NODE_CENTER_Y_OFFSET, { duration: 300, zoom: 1 } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/graph/OntologyGraph.tsx` around lines 202 - 213, The viewport centering uses hardcoded offsets (x + 90, y + 22); update the logic in the effect that references lastExpandedIdRef, layoutNodes, and setCenter to compute offsets from explicit node size constants (e.g., NODE_WIDTH and NODE_HEIGHT) or derived values (NODE_WIDTH / 2, NODE_HEIGHT / 2) instead of magic numbers, then call setCenter with target.position.x + halfWidth and target.position.y + halfHeight so the centering stays correct if node dimensions change.lib/hooks/useGraphData.ts (1)
24-52:mergeExpansionsallocates new Sets on each iteration.For large expansion histories, rebuilding
existingNodeIdsandexistingEdgeIdson every loop iteration creates allocation pressure. Consider building a single cumulative Set outside the loop.♻️ Suggested optimization
function mergeExpansions( base: EntityGraphResponse, expansions: EntityGraphResponse[], ): EntityGraphResponse { if (expansions.length === 0) return base; - let result = base; - for (const expansion of expansions) { - const existingNodeIds = new Set(result.nodes.map((n) => n.id)); - const existingEdgeIds = new Set(result.edges.map((e) => e.id)); - result = { - ...result, - nodes: [ - ...result.nodes, - ...expansion.nodes.filter((n) => !existingNodeIds.has(n.id)), - ], - edges: [ - ...result.edges, - ...expansion.edges.filter((e) => !existingEdgeIds.has(e.id)), - ], - truncated: result.truncated || expansion.truncated, - total_concept_count: Math.max( - result.total_concept_count, - expansion.total_concept_count, - ), - }; - } - return result; + const nodeIds = new Set(base.nodes.map((n) => n.id)); + const edgeIds = new Set(base.edges.map((e) => e.id)); + const newNodes = [...base.nodes]; + const newEdges = [...base.edges]; + let truncated = base.truncated; + let totalCount = base.total_concept_count; + + for (const expansion of expansions) { + for (const node of expansion.nodes) { + if (!nodeIds.has(node.id)) { + nodeIds.add(node.id); + newNodes.push(node); + } + } + for (const edge of expansion.edges) { + if (!edgeIds.has(edge.id)) { + edgeIds.add(edge.id); + newEdges.push(edge); + } + } + truncated = truncated || expansion.truncated; + totalCount = Math.max(totalCount, expansion.total_concept_count); + } + + return { ...base, nodes: newNodes, edges: newEdges, truncated, total_concept_count: totalCount }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks/useGraphData.ts` around lines 24 - 52, The function mergeExpansions currently recreates existingNodeIds and existingEdgeIds inside each loop iteration which causes allocation overhead; refactor mergeExpansions to create cumulative Sets for node IDs and edge IDs before the loop (seeded from base.nodes and base.edges), then for each expansion filter using those cumulative sets and add newly accepted node/edge ids into the same Sets as you append them to result.nodes/result.edges; preserve the existing logic for truncated and total_concept_count (OR for truncated, Math.max for total_concept_count) while avoiding per-iteration Set recreation.__tests__/lib/graph/useELKLayout.test.ts (2)
126-137: Test validates layout runs but doesn't assert width calculation.The test mutates the label but only asserts that the node exists. Consider verifying the mock received the expected width to ensure the dynamic sizing formula works correctly.
💡 Example enhancement
// The mock could capture the input width and assert: // label = "A very long label for testing width" (36 chars) // expected width = max(180, 36 * 7.5 + 32) = max(180, 302) = 302🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/graph/useELKLayout.test.ts` around lines 126 - 137, The test only checks that a node exists but doesn't verify that dynamic width was computed from label length; update the test for useELKLayout so after calling result.current.runLayout(data) you assert the computed node width equals the expected value (use the formula used by the layout code, e.g., expectedWidth = Math.max(180, label.length * 7.5 + 32) for label "A very long label for testing width" → 302) — either assert result.current.nodes[0].width === expectedWidth or, if a mock ELK layout function is used, assert the mock was called with node.size.width equal to expectedWidth.
62-64: Consider resettingdeferredLayoutinbeforeEachfor better test isolation.If the
isLayoutingtest fails before reaching line 221,deferredLayoutremains set, potentially affecting subsequent tests. Adding cleanup ensures test independence.🔧 Suggested fix
describe("useELKLayout", () => { beforeEach(() => { vi.clearAllMocks(); + deferredLayout = null; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/graph/useELKLayout.test.ts` around lines 62 - 64, The beforeEach currently only calls vi.clearAllMocks(); also reset the test-scoped deferredLayout to clear any leftover state between tests: inside the existing beforeEach add a line that sets deferredLayout = undefined (or null) so any pending deferredLayout from a failed run is cleared; if deferredLayout is an object with resolve/reject methods, additionally call its resolve/reject or set it to undefined to avoid dangling promises. Ensure you update the beforeEach that references vi.clearAllMocks() and modify the module-scoped deferredLayout variable used in the isLayouting tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@__tests__/lib/graph/useELKLayout.test.ts`:
- Around line 126-137: The test only checks that a node exists but doesn't
verify that dynamic width was computed from label length; update the test for
useELKLayout so after calling result.current.runLayout(data) you assert the
computed node width equals the expected value (use the formula used by the
layout code, e.g., expectedWidth = Math.max(180, label.length * 7.5 + 32) for
label "A very long label for testing width" → 302) — either assert
result.current.nodes[0].width === expectedWidth or, if a mock ELK layout
function is used, assert the mock was called with node.size.width equal to
expectedWidth.
- Around line 62-64: The beforeEach currently only calls vi.clearAllMocks();
also reset the test-scoped deferredLayout to clear any leftover state between
tests: inside the existing beforeEach add a line that sets deferredLayout =
undefined (or null) so any pending deferredLayout from a failed run is cleared;
if deferredLayout is an object with resolve/reject methods, additionally call
its resolve/reject or set it to undefined to avoid dangling promises. Ensure you
update the beforeEach that references vi.clearAllMocks() and modify the
module-scoped deferredLayout variable used in the isLayouting tests.
In `@components/graph/OntologyGraph.tsx`:
- Around line 202-213: The viewport centering uses hardcoded offsets (x + 90, y
+ 22); update the logic in the effect that references lastExpandedIdRef,
layoutNodes, and setCenter to compute offsets from explicit node size constants
(e.g., NODE_WIDTH and NODE_HEIGHT) or derived values (NODE_WIDTH / 2,
NODE_HEIGHT / 2) instead of magic numbers, then call setCenter with
target.position.x + halfWidth and target.position.y + halfHeight so the
centering stays correct if node dimensions change.
In `@lib/hooks/useGraphData.ts`:
- Around line 24-52: The function mergeExpansions currently recreates
existingNodeIds and existingEdgeIds inside each loop iteration which causes
allocation overhead; refactor mergeExpansions to create cumulative Sets for node
IDs and edge IDs before the loop (seeded from base.nodes and base.edges), then
for each expansion filter using those cumulative sets and add newly accepted
node/edge ids into the same Sets as you append them to
result.nodes/result.edges; preserve the existing logic for truncated and
total_concept_count (OR for truncated, Math.max for total_concept_count) while
avoiding per-iteration Set recreation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a980cbe5-5f42-43f7-a528-780dc6c14ae4
📒 Files selected for processing (9)
.planning/features/entity-graph-port/CONTEXT.md__tests__/components/graph/OntologyGraph.test.tsx__tests__/lib/graph/useELKLayout.test.ts__tests__/lib/hooks/useGraphData.test.tscomponents/editor/standard/StandardEditorLayout.tsxcomponents/graph/OntologyGraph.tsxcomponents/graph/OntologyNode.tsxlib/graph/useELKLayout.tslib/hooks/useGraphData.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- components/editor/standard/StandardEditorLayout.tsx
- tests/lib/hooks/useGraphData.test.ts
- tests/components/graph/OntologyGraph.test.tsx
JohnRDOrazio
force-pushed
the
entity-graph-pr
branch
from
21b14fb to
230cd07
Compare
Fix react/display-name error in test QueryClient wrapper by assigning displayName. Refactor useGraphData to store expansions separately and merge via useMemo, avoiding setState-in-render from ref access during render phase. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The useEffect that resets expansion tracking depended on query.data, causing expansions to be cleared on background refetches (staleTime expiry, window refocus). Now depends on the actual graph input props (projectId, focusIri, branch, showDescendants, resetKey) so expansions are only cleared when the graph context truly changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Always fetch descendantsDepth: 1 so direct children appear on initial graph load. Rename showDescendants to showAllDescendants — the toggle now controls whether to fetch deeper descendants (depth 2) rather than toggling descendants on/off entirely. Update button labels and tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…erges useEffect runs after paint, so a promise .then() microtask from expandNode could execute between render and the effect, passing the stale epoch guard. useLayoutEffect runs synchronously in the commit phase, closing this race window. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When graphData becomes null or has no nodes, the layout effect returned early but left the previous React Flow state intact, showing a stale graph. Now explicitly calls setNodes([]) and setEdges([]) to clear the canvas. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
OntologyGraph maintained its own expandedNodes ref and reset effect that duplicated the deduplication already handled inside useGraphData's expandNode. Simplified handleNodeClick to always delegate to expandNode and removed the unused useRef import. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The catch handler for runLayout was empty, making layout failures invisible. Now logs the error with console.error for debugging. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Single click on a node now waits 200ms before calling expandNode. If a double click follows within that window, the expand is cancelled and onNavigateToClass fires instead. This prevents unintended expansion when the user intends to navigate. Timer is cleaned up on unmount. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add explicit tests for the "Computing layout..." indicator when isLayouting is true, and verify that accessToken is forwarded to useGraphData. Made useELKLayout mock's isLayouting value overridable per test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The useQuery enabled condition only checked focusIri, allowing unauthorized requests when the session token wasn't yet available. Now requires both focusIri and accessToken. Added test for missing accessToken case. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Handles were hardcoded to Position.Top/Bottom which breaks edge anchoring in left-right layouts. Now reads layoutDirection from node data and uses Position.Left/Right for LR layout. The direction is passed through from useELKLayout when building node data. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add normalizeEdgeType helper matching the existing normalizeNodeType pattern. Unknown edge types fall back to subClassOf instead of passing through an invalid enum value to OntologyEdge. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the static import of EntityGraphModal with next/dynamic so the modal code is only loaded when the user opens the full-screen graph view, matching the existing pattern used for OntologyGraph. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous test awaited runLayout to completion so it never saw isLayouting=true. Now uses a deferred promise in the ELK mock to assert isLayouting=true while layout is pending, then resolves and verifies it returns to false. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
After clicking a node to expand, the ELK relayout could move the clicked node off-screen. Now tracks the last expanded node ID and smoothly centers the viewport on it (300ms transition) once layout finishes. Wraps OntologyGraph in ReactFlowProvider to enable useReactFlow's setCenter. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ELKLayout Add tests for LR layout handle positions, edge style toggle, and normalization fallback branches (secondary_root, unknown node_type, unknown edge_type). Brings useELKLayout to 100% statement coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Optimize mergeExpansions with cumulative Sets instead of recreating per iteration - Replace magic numbers in viewport centering with NODE_WIDTH/NODE_HEIGHT constants exported from useELKLayout - Assert dynamic node width via captured ELK mock call in test - Reset deferredLayout in beforeEach to prevent stale test state Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Focus the first focusable element on open instead of the dialog container. Also handle Shift+Tab when activeElement is the dialog container itself (tabIndex=-1), wrapping to the last focusable element. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
If graphData was cleared while a previous runLayout was still resolving, the stale layoutNodes/layoutEdges would be reapplied by the sync effect. Now checks graphData is non-null before syncing layout results. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Assert Authorization header is sent/omitted based on token presence - Verify stale expandNode responses are discarded after resetGraph - Update EntityGraphModal focus test for first-focusable-element behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Reset capturedOnNavigateToClass in beforeEach to prevent cross-test leaks - Focus a middle element in the focus trap test instead of the first - Assert reset payload (focus_label) instead of just non-null graphData - Use objectContaining for options assertion to avoid pinning undefined keys Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ow markers Merge visual enhancements with colleague's entity-graph-pr work: - Add secondary_root node type with distinct slate styling and legend entry - Pin root and secondary_root nodes to first ELK layer - Remove smoothstep/bezier toggle, use bezier-only edges - Add per-edge-type markerEnd config (subClassOf gets arrow-slate SVG marker) - Add secondary_root to MiniMap color mapping - Update tests: remove smoothstep mocks, add secondary_root + layer constraint tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
JohnRDOrazio
force-pushed
the
entity-graph-pr
branch
from
a938529 to
6fd5d83
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/graph/useELKLayout.ts`:
- Around line 112-122: The marker rendering condition uses the raw e.edge_type,
causing edges whose type is normalized to "subClassOf" (via normalizeEdgeType)
to miss the arrow; update the mapping in layoutedEdges so you compute the
normalized type once (e.g., const normalized = normalizeEdgeType(e.edge_type)
inside the map) and use that normalized value both for the marker condition and
for data.edgeType (or call normalizeEdgeType(e.edge_type) in the marker
conditional), ensuring markerEnd is applied when normalized === "subClassOf".
🪄 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
Run ID: 223974e3-7682-4f06-955b-c28c2012fe39
📒 Files selected for processing (24)
__tests__/components/graph/EntityGraphModal.test.tsx__tests__/components/graph/OntologyEdge.test.tsx__tests__/components/graph/OntologyGraph.test.tsx__tests__/components/graph/OntologyNode.test.tsx__tests__/lib/api/graph.test.ts__tests__/lib/graph/buildGraphData.test.ts__tests__/lib/graph/elkLayout.test.ts__tests__/lib/graph/useELKLayout.test.ts__tests__/lib/graph/utils.test.ts__tests__/lib/hooks/useGraphData.test.tscomponents/editor/developer/DeveloperEditorLayout.tsxcomponents/editor/standard/StandardEditorLayout.tsxcomponents/graph/EntityGraphModal.tsxcomponents/graph/OntologyEdge.tsxcomponents/graph/OntologyGraph.tsxcomponents/graph/OntologyNode.tsxlib/api/graph.tslib/graph/buildGraphData.tslib/graph/elkLayout.tslib/graph/types.tslib/graph/useELKLayout.tslib/graph/utils.tslib/hooks/useGraphData.tslib/stores/editorModeStore.ts
💤 Files with no reviewable changes (4)
- tests/lib/graph/buildGraphData.test.ts
- tests/lib/graph/elkLayout.test.ts
- lib/graph/elkLayout.ts
- lib/graph/buildGraphData.ts
✅ Files skipped from review due to trivial changes (3)
- components/graph/OntologyEdge.tsx
- lib/graph/types.ts
- tests/lib/graph/utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/components/graph/OntologyNode.test.tsx
- lib/graph/utils.ts
- lib/stores/editorModeStore.ts
- components/editor/developer/DeveloperEditorLayout.tsx
- tests/lib/api/graph.test.ts
- tests/components/graph/OntologyEdge.test.tsx
- tests/components/graph/EntityGraphModal.test.tsx
- lib/hooks/useGraphData.ts
The marker condition checked the raw backend edge_type, so edges normalized to "subClassOf" via normalizeEdgeType could miss the arrow. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
Contributor
Author
|
Peer review summary (companion review on the backend at CatholicOS/ontokit-api#37): Findings: 0 BLOCKING + 3 HIGH + 5 MEDIUM + 5 LOW. All 2519 tests pass.
The PR is solid: the architecture pivot from client-side graph building to server-side BFS is the right call, concurrency in 🤖 Review assisted by Claude Code |
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
2 participants
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.
Summary
GET /api/v1/projects/{id}/ontology/classes/graph?class_iri=...) — graph data now comes from the backend instead of being computed in the browseruseELKLayout) with dynamic node widths based on label length, replacing the old staticelkLayout.tsexpandNode(), double-click to navigateuseQuerywith caching, deduplication, and cancellationAPI route
The frontend calls:
class_iriis passed as a query parameter (not a path segment)Authorization: Bearerheaderancestors_depth=1&descendants_depth=1&max_nodes=50Requires backend: CatholicOS/ontokit-api#37
What changed
buildGraphData.tsparsed tree structuregraphApi.getEntityGraph()calls backend BFSuseEffectwith cancelled flaguseQuerywith staleTime cachingelkLayout.tsstatic wrapperuseELKLayouthook with dynamic widthsEntityGraphModaloverlayuseGraphDatahookascastnormalizeNodeType()with runtime validationbuildGraphData.test.tsuseELKLayout.test.ts,utils.test.ts,useGraphData.test.ts,OntologyEdge.test.tsKey fixes since initial PR
class_irias query param (not path segment)Verification
Test plan
Closes #81
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UI/UX Improvements