Port entity graph to server-side BFS endpoint#88
Open
damienriehl wants to merge 2 commits intomainfrom
Open
Conversation
📝 WalkthroughWalkthroughThis PR ports the Entity Graph feature from folio-mapper to ontokit-web as a full-stack refactor. It replaces client-side graph building with a server-driven BFS API endpoint, introduces an ELK layout hook, adds a full-screen modal component, and removes legacy graph construction code while updating editor integrations. Changes
Sequence DiagramsequenceDiagram
participant App as React Component
participant Hook as useGraphData Hook
participant API as graphApi Client
participant Server as Backend API
participant ELK as useELKLayout Hook
participant ReactFlow as React Flow
App->>Hook: focusIri, projectId, branch
Hook->>API: getEntityGraph(projectId, focusIri, options)
API->>Server: GET /api/v1/projects/{id}/ontology/graph/{iri}
Server-->>API: EntityGraphResponse{nodes, edges, truncated}
API-->>Hook: graphData
Hook->>Hook: Update state with graphData
Hook-->>App: graphData, expandNode, resetGraph
App->>ELK: runLayout(graphData, direction)
ELK->>ELK: Convert to ELK format
ELK->>ELK: elk.layout() with layered algorithm
ELK->>ELK: Map coordinates back to React Flow nodes
ELK-->>App: nodes[], edges[], isLayouting
App->>ReactFlow: Update with positioned nodes/edges
ReactFlow-->>App: Rendered graph
App->>App: User clicks unexplored node
App->>Hook: expandNode(nodeIri)
Hook->>API: getEntityGraph(projectId, nodeIri, {ancestorsDepth: 1, descendantsDepth: 1})
API->>Server: GET /api/v1/projects/{id}/ontology/graph/{nodeIri}
Server-->>API: EntityGraphResponse (1-hop neighbors)
API-->>Hook: Merge nodes/edges into existing graphData
Hook-->>App: Updated graphData with new neighbors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
Comment on lines
+43
to
+45
| return ( | ||
| <div className="fixed inset-0 z-[60] flex items-center justify-center bg-black/40"> | ||
| <div className="flex h-[97vh] w-[98vw] flex-col overflow-hidden rounded-xl bg-white shadow-2xl dark:bg-slate-900"> |
There was a problem hiding this comment.
Add real dialog semantics here.
This fullscreen modal is rendered as plain divs, so assistive tech is never told a dialog opened and keyboard focus can keep moving through the obscured editor behind it. Please add role="dialog", aria-modal="true", wire aria-labelledby to the title, and move focus into the modal on open.
Also applies to: 62-68
🤖 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 - 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.
Comment on lines
+183
to
+195
| // Progressive expansion: click to expand or navigate | ||
| const expandedNodes = useMemo(() => new Set<string>(focusIri ? [focusIri] : []), [focusIri]); | ||
|
|
||
| const handleNodeClick: NodeMouseHandler = useCallback( | ||
| (_event, node) => { | ||
| if (expandedNodes.has(node.id)) { | ||
| onNavigateToClass?.(node.id); | ||
| return; | ||
| } | ||
| expandedNodes.add(node.id); | ||
| expandNode(node.id); | ||
| }, | ||
| [onNavigateToClass], | ||
| [expandedNodes, expandNode, onNavigateToClass], |
There was a problem hiding this comment.
Keep navigation on double-click only.
expandedNodes is seeded with focusIri, so the focus node already navigates on the first single click. After any expansion, later single clicks also navigate, and the second click of a double-click can hit this branch before onNodeDoubleClick runs. That breaks the advertised "click to expand / double-click to navigate" interaction, and in modal mode it can close the viewer on a single click.
🖱️ Minimal click-handler 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],
);Also applies to: 198-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/graph/OntologyGraph.tsx` around lines 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.
Comment on lines
+47
to
+63
| getEntityGraph: ( | ||
| projectId: string, | ||
| classIri: string, | ||
| options: FetchGraphOptions = {}, | ||
| ) => | ||
| api.get<EntityGraphResponse>( | ||
| `/api/v1/projects/${projectId}/ontology/graph/${encodeURIComponent(classIri)}`, | ||
| { | ||
| params: { | ||
| branch: options.branch, | ||
| ancestors_depth: options.ancestorsDepth, | ||
| descendants_depth: options.descendantsDepth, | ||
| max_nodes: options.maxNodes, | ||
| include_see_also: options.includeSeeAlso, | ||
| }, | ||
| }, | ||
| ), |
There was a problem hiding this comment.
Thread session.accessToken into graphApi.getEntityGraph().
api.get() only sends auth headers when they are provided explicitly, but this helper has no way to receive or forward the bearer token. In authenticated projects that means the new graph fetches go out anonymously, so protected graph endpoints will fail even though the editor session is already logged in. Callers will need to pass the session token through this API.
🔐 Suggested shape
export const graphApi = {
getEntityGraph: (
projectId: string,
classIri: string,
+ accessToken: string,
options: FetchGraphOptions = {},
) =>
api.get<EntityGraphResponse>(
`/api/v1/projects/${projectId}/ontology/graph/${encodeURIComponent(classIri)}`,
{
+ headers: {
+ Authorization: `Bearer ${accessToken}`,
+ },
params: {
branch: options.branch,
ancestors_depth: options.ancestorsDepth,
descendants_depth: options.descendantsDepth,
max_nodes: options.maxNodes,
include_see_also: options.includeSeeAlso,
},
},
),
};As per coding guidelines, Authentication is handled via NextAuth.js v5 with Zitadel OIDC provider. The auth.ts file at root handles token refresh and session management. Access tokens must be passed to API calls via session.accessToken.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| getEntityGraph: ( | |
| projectId: string, | |
| classIri: string, | |
| options: FetchGraphOptions = {}, | |
| ) => | |
| api.get<EntityGraphResponse>( | |
| `/api/v1/projects/${projectId}/ontology/graph/${encodeURIComponent(classIri)}`, | |
| { | |
| params: { | |
| branch: options.branch, | |
| ancestors_depth: options.ancestorsDepth, | |
| descendants_depth: options.descendantsDepth, | |
| max_nodes: options.maxNodes, | |
| include_see_also: options.includeSeeAlso, | |
| }, | |
| }, | |
| ), | |
| getEntityGraph: ( | |
| projectId: string, | |
| classIri: string, | |
| accessToken: string, | |
| options: FetchGraphOptions = {}, | |
| ) => | |
| api.get<EntityGraphResponse>( | |
| `/api/v1/projects/${projectId}/ontology/graph/${encodeURIComponent(classIri)}`, | |
| { | |
| headers: { | |
| Authorization: `Bearer ${accessToken}`, | |
| }, | |
| params: { | |
| branch: options.branch, | |
| ancestors_depth: options.ancestorsDepth, | |
| descendants_depth: options.descendantsDepth, | |
| max_nodes: options.maxNodes, | |
| include_see_also: options.includeSeeAlso, | |
| }, | |
| }, | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/api/graph.ts` around lines 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.
Comment on lines
+27
to
+100
| const runLayout = useCallback( | ||
| async (data: EntityGraphResponse, direction: LayoutDirection = "TB") => { | ||
| setIsLayouting(true); | ||
|
|
||
| try { | ||
| const ELK = (await import("elkjs/lib/elk.bundled.js")).default; | ||
| const elk = new ELK(); | ||
|
|
||
| const elkNodes = data.nodes.map((n) => ({ | ||
| id: n.id, | ||
| width: Math.max(NODE_WIDTH, n.label.length * 7.5 + 32), | ||
| height: NODE_HEIGHT, | ||
| })); | ||
|
|
||
| const elkEdges = data.edges.map((e) => ({ | ||
| id: e.id, | ||
| sources: [e.source], | ||
| targets: [e.target], | ||
| })); | ||
|
|
||
| const elkGraph = await elk.layout({ | ||
| id: "root", | ||
| layoutOptions: { | ||
| "elk.algorithm": "layered", | ||
| "elk.direction": direction === "TB" ? "DOWN" : "RIGHT", | ||
| "elk.spacing.nodeNode": "40", | ||
| "elk.layered.spacing.nodeNodeBetweenLayers": "70", | ||
| "elk.layered.crossingMinimization.strategy": "LAYER_SWEEP", | ||
| "elk.edgeRouting": "SPLINES", | ||
| "elk.layered.nodePlacement.strategy": "BRANDES_KOEPF", | ||
| }, | ||
| children: elkNodes, | ||
| edges: elkEdges, | ||
| }); | ||
|
|
||
| const nodeMap = new Map(data.nodes.map((n) => [n.id, n])); | ||
| const layoutedNodes: Node<OntologyNodeData>[] = (elkGraph.children ?? []).map( | ||
| (elkNode) => { | ||
| const backendNode = nodeMap.get(elkNode.id)!; | ||
| return { | ||
| id: elkNode.id, | ||
| type: "ontologyNode", | ||
| position: { x: elkNode.x ?? 0, y: elkNode.y ?? 0 }, | ||
| data: { | ||
| label: backendNode.label, | ||
| nodeType: (backendNode.node_type || "class") as GraphNodeType, | ||
| childCount: backendNode.child_count ?? undefined, | ||
| deprecated: false, | ||
| isExpanded: false, | ||
| }, | ||
| }; | ||
| }, | ||
| ); | ||
|
|
||
| const edgeMarkers: Partial<Record<GraphEdgeType, { type: "arrowclosed"; color: string }>> = { | ||
| subClassOf: { type: "arrowclosed", color: "#94a3b8" }, | ||
| }; | ||
|
|
||
| const layoutedEdges: Edge<OntologyEdgeData>[] = data.edges.map((e) => ({ | ||
| id: e.id, | ||
| source: e.source, | ||
| target: e.target, | ||
| type: "ontologyEdge", | ||
| markerEnd: edgeMarkers[e.edge_type as GraphEdgeType], | ||
| data: { | ||
| edgeType: e.edge_type as GraphEdgeType, | ||
| }, | ||
| })); | ||
|
|
||
| setNodes(layoutedNodes); | ||
| setEdges(layoutedEdges); | ||
| } finally { | ||
| setIsLayouting(false); | ||
| } |
There was a problem hiding this comment.
Guard runLayout() against stale completions.
Rapid direction changes or graph expansions can leave multiple async layouts in flight. Every completion currently calls setNodes/setEdges, so an older layout can overwrite the newest graph orientation or content.
🔁 Suggested stale-run guard
-"use client";
+"use client";
-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 - 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.
Comment on lines
+27
to
63
| const [graphData, setGraphData] = useState<EntityGraphResponse | null>(null); | ||
| const [isLoading, setIsLoading] = useState(false); | ||
| const resolvedNodesRef = useRef<Map<string, OWLClassDetail>>(new Map()); | ||
| // IRIs that failed getClassDetail (non-class entities) — don't retry | ||
| const failedIrisRef = useRef<Set<string>>(new Set()); | ||
| // Labels + entity types discovered for non-class entities via search API | ||
| const nonClassLabelsRef = useRef<Map<string, { label: string; entityType: string }>>(new Map()); | ||
| const [resolvedCount, setResolvedCount] = useState(0); | ||
|
|
||
| const fetchDetail = useCallback( | ||
| async (iri: string): Promise<OWLClassDetail | null> => { | ||
| if (resolvedNodesRef.current.has(iri)) { | ||
| return resolvedNodesRef.current.get(iri)!; | ||
| } | ||
| if (failedIrisRef.current.has(iri)) return null; | ||
| if (resolvedNodesRef.current.size >= MAX_RESOLVED_NODES) return null; | ||
| try { | ||
| const detail = await projectOntologyApi.getClassDetail( | ||
| projectId, | ||
| iri, | ||
| accessToken, | ||
| branch, | ||
| ); | ||
| resolvedNodesRef.current.set(iri, detail); | ||
| return detail; | ||
| } catch { | ||
| failedIrisRef.current.add(iri); | ||
| return null; | ||
| } | ||
| }, | ||
| [projectId, accessToken, branch], | ||
| ); | ||
|
|
||
| const fetchNeighbors = useCallback( | ||
| async (iris: string[]): Promise<string[]> => { | ||
| const unresolved = iris.filter( | ||
| (iri) => | ||
| !resolvedNodesRef.current.has(iri) && | ||
| !failedIrisRef.current.has(iri) && | ||
| resolvedNodesRef.current.size < MAX_RESOLVED_NODES, | ||
| ); | ||
| if (unresolved.length === 0) return []; | ||
|
|
||
| const results = await Promise.allSettled( | ||
| unresolved.map((iri) => fetchDetail(iri)), | ||
| ); | ||
|
|
||
| const newNeighborIris: string[] = []; | ||
| for (const result of results) { | ||
| if (result.status === "fulfilled" && result.value) { | ||
| const detail = result.value; | ||
| for (const parentIri of detail.parent_iris) { | ||
| if (!resolvedNodesRef.current.has(parentIri)) { | ||
| newNeighborIris.push(parentIri); | ||
| } | ||
| } | ||
| for (const eqIri of detail.equivalent_iris ?? []) { | ||
| if (!resolvedNodesRef.current.has(eqIri)) { | ||
| newNeighborIris.push(eqIri); | ||
| } | ||
| } | ||
| for (const djIri of detail.disjoint_iris ?? []) { | ||
| if (!resolvedNodesRef.current.has(djIri)) { | ||
| newNeighborIris.push(djIri); | ||
| } | ||
| } | ||
| for (const seeAlsoIri of getSeeAlsoIris(detail)) { | ||
| if (!resolvedNodesRef.current.has(seeAlsoIri)) { | ||
| newNeighborIris.push(seeAlsoIri); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return [...new Set(newNeighborIris)]; | ||
| }, | ||
| [fetchDetail], | ||
| ); | ||
|
|
||
| /** | ||
| * Resolve full ancestry for all currently resolved class nodes. | ||
| * Calls the ancestors endpoint for each class, adds missing ancestor nodes. | ||
| */ | ||
| const resolveAncestry = useCallback(async () => { | ||
| const classIris = [...resolvedNodesRef.current.keys()]; | ||
| const MAX_ANCESTOR_CALLS = 50; | ||
| let callCount = 0; | ||
|
|
||
| // Collect all ancestor IRIs we need to resolve | ||
| const ancestorIrisToResolve = new Set<string>(); | ||
|
|
||
| await Promise.allSettled( | ||
| classIris.map(async (iri) => { | ||
| if (callCount >= MAX_ANCESTOR_CALLS) return; | ||
| callCount++; | ||
| try { | ||
| const response = await projectOntologyApi.getClassAncestors( | ||
| projectId, | ||
| iri, | ||
| accessToken, | ||
| branch, | ||
| ); | ||
| for (const node of response.nodes) { | ||
| if (!resolvedNodesRef.current.has(node.iri) && !failedIrisRef.current.has(node.iri)) { | ||
| ancestorIrisToResolve.add(node.iri); | ||
| } | ||
| } | ||
| } catch { | ||
| // Ancestors endpoint failed — skip this node | ||
| } | ||
| }), | ||
| ); | ||
|
|
||
| // Fetch full details for discovered ancestors | ||
| if (ancestorIrisToResolve.size > 0) { | ||
| await Promise.allSettled( | ||
| [...ancestorIrisToResolve].map((iri) => fetchDetail(iri)), | ||
| ); | ||
| } | ||
| }, [projectId, accessToken, branch, fetchDetail]); | ||
|
|
||
| /** Collect all IRIs referenced by resolved nodes that are still unresolved. */ | ||
| const getUnresolvedReferenced = useCallback((): string[] => { | ||
| const referenced = new Set<string>(); | ||
| for (const detail of resolvedNodesRef.current.values()) { | ||
| for (const iri of detail.parent_iris) referenced.add(iri); | ||
| for (const iri of detail.equivalent_iris ?? []) referenced.add(iri); | ||
| for (const iri of detail.disjoint_iris ?? []) referenced.add(iri); | ||
| for (const iri of getSeeAlsoIris(detail)) referenced.add(iri); | ||
| } | ||
| return [...referenced].filter( | ||
| (iri) => !resolvedNodesRef.current.has(iri) && !failedIrisRef.current.has(iri), | ||
| ); | ||
| }, []); | ||
|
|
||
| /** | ||
| * Resolve labels for non-class entities (individuals, properties) via search API. | ||
| * Searches by each IRI's local name and matches by exact IRI in results. | ||
| */ | ||
| const resolveNonClassLabels = useCallback(async () => { | ||
| const needLabels = [...failedIrisRef.current].filter( | ||
| (iri) => !nonClassLabelsRef.current.has(iri), | ||
| ); | ||
| if (needLabels.length === 0) return; | ||
|
|
||
| await Promise.allSettled( | ||
| needLabels.map(async (iri) => { | ||
| try { | ||
| const localName = getLocalName(iri); | ||
| const response = await projectOntologyApi.searchEntities( | ||
| projectId, | ||
| localName, | ||
| accessToken, | ||
| branch, | ||
| ); | ||
| const match = response.results.find((r) => r.iri === iri); | ||
| if (match) { | ||
| nonClassLabelsRef.current.set(iri, { | ||
| label: match.label || getLocalName(iri), | ||
| entityType: match.entity_type, | ||
| }); | ||
| } | ||
| } catch { | ||
| // Search failed for this IRI — label will fall back to getLocalName | ||
| } | ||
| }), | ||
| ); | ||
| }, [projectId, accessToken, branch]); | ||
| const [showDescendants, setShowDescendants] = useState(false); | ||
| const expandedNodes = useRef(new Set<string>()); | ||
|
|
||
| const buildGraph = useCallback( | ||
| (focus: string) => { | ||
| // Merge all label sources: caller hints + non-class entity labels | ||
| let mergedHints = labelHints; | ||
| let entityTypes: Map<string, string> | undefined; | ||
| if (nonClassLabelsRef.current.size > 0) { | ||
| mergedHints = new Map(labelHints); | ||
| entityTypes = new Map(); | ||
| for (const [iri, info] of nonClassLabelsRef.current) { | ||
| if (!mergedHints.has(iri)) mergedHints.set(iri, info.label); | ||
| entityTypes.set(iri, info.entityType); | ||
| } | ||
| } | ||
| const data = buildGraphFromClassDetail(focus, resolvedNodesRef.current, mergedHints, entityTypes); | ||
| setGraphData(data); | ||
| setResolvedCount(resolvedNodesRef.current.size); | ||
| }, | ||
| [labelHints], | ||
| ); | ||
|
|
||
| // Initial load when focus changes | ||
| // Fetch graph from backend BFS endpoint | ||
| useEffect(() => { | ||
| if (!focusIri || !accessToken) { | ||
| if (!focusIri) { | ||
| setGraphData(null); | ||
| return; | ||
| } | ||
|
|
||
| let cancelled = false; | ||
|
|
||
| async function loadGraph() { | ||
| setIsLoading(true); | ||
| resolvedNodesRef.current = new Map(); | ||
| failedIrisRef.current = new Set(); | ||
| nonClassLabelsRef.current = new Map(); | ||
|
|
||
| try { | ||
| // Depth 0: focus node | ||
| const focusDetail = await fetchDetail(focusIri!); | ||
| if (cancelled || !focusDetail) { | ||
| if (!cancelled) { | ||
| // Even without detail, show a single-node graph | ||
| buildGraph(focusIri!); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // Depth 1: immediate neighbors (including seeAlso/isDefinedBy targets) | ||
| const depth1Iris = [ | ||
| ...focusDetail.parent_iris, | ||
| ...(focusDetail.equivalent_iris ?? []), | ||
| ...(focusDetail.disjoint_iris ?? []), | ||
| ...getSeeAlsoIris(focusDetail), | ||
| ]; | ||
| const depth2Candidates = await fetchNeighbors(depth1Iris); | ||
| if (cancelled) return; | ||
|
|
||
| // Depth 2: neighbors of neighbors (if initialDepth >= 2) | ||
| if (initialDepth >= 2 && depth2Candidates.length > 0) { | ||
| await fetchNeighbors(depth2Candidates); | ||
| if (cancelled) return; | ||
| } | ||
|
|
||
| // Label resolution pass: fetch details for any remaining unresolved | ||
| // relationship targets so graph nodes show labels instead of opaque IDs | ||
| const unresolved = getUnresolvedReferenced(); | ||
| if (unresolved.length > 0) { | ||
| await Promise.allSettled(unresolved.map((iri) => fetchDetail(iri))); | ||
| if (cancelled) return; | ||
| } | ||
|
|
||
| // Ancestry resolution: trace all displayed classes back to root | ||
| await resolveAncestry(); | ||
| if (cancelled) return; | ||
|
|
||
| // Resolve labels for non-class entities (individuals/properties) | ||
| if (failedIrisRef.current.size > 0) { | ||
| await resolveNonClassLabels(); | ||
| if (cancelled) return; | ||
| } | ||
|
|
||
| buildGraph(focusIri!); | ||
| } finally { | ||
| setIsLoading(true); | ||
| expandedNodes.current = new Set([focusIri]); | ||
|
|
||
| graphApi | ||
| .getEntityGraph(projectId, focusIri, { | ||
| branch, | ||
| ancestorsDepth: 5, | ||
| descendantsDepth: showDescendants ? 2 : 0, | ||
| }) | ||
| .then((data) => { | ||
| if (!cancelled) setGraphData(data); | ||
| }) | ||
| .catch(() => { | ||
| if (!cancelled) setGraphData(null); | ||
| }) | ||
| .finally(() => { | ||
| if (!cancelled) setIsLoading(false); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| loadGraph(); | ||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [focusIri, accessToken, branch, fetchDetail, fetchNeighbors, getUnresolvedReferenced, resolveAncestry, resolveNonClassLabels, buildGraph, initialDepth]); | ||
| }, [focusIri, projectId, branch, showDescendants]); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file mentioned in the review
fd -t f "useGraphData.ts" 2>/dev/nullRepository: CatholicOS/ontokit-web
Length of output: 91
🏁 Script executed:
# Check if React Query is in the project dependencies
find . -name "package.json" -type f | head -5 | xargs cat 2>/dev/null | grep -A 5 -B 5 "react-query\|@tanstack/react-query" || echo "Checking package-lock or yarn.lock"Repository: CatholicOS/ontokit-web
Length of output: 447
🏁 Script executed:
# Search for useQuery usage patterns in other hooks
fd -t f "use.*\.ts" lib/hooks/ 2>/dev/null | head -10Repository: CatholicOS/ontokit-web
Length of output: 364
🏁 Script executed:
# Read the full useGraphData.ts file to verify the pattern
cat -n lib/hooks/useGraphData.tsRepository: CatholicOS/ontokit-web
Length of output: 4192
🏁 Script executed:
# Check a comparable hook to see React Query patterns
cat -n lib/hooks/useIriLabels.ts | head -80Repository: CatholicOS/ontokit-web
Length of output: 3573
🏁 Script executed:
# Search for useQuery or useMutation usage in the codebase to see if React Query is actually used
rg "useQuery|useMutation|useInfiniteQuery" --type ts --type tsx -cRepository: CatholicOS/ontokit-web
Length of output: 93
🏁 Script executed:
# More specific search for React Query imports
rg "from.*@tanstack/react-query|from.*react-query" --type ts --type tsxRepository: CatholicOS/ontokit-web
Length of output: 93
🏁 Script executed:
# Search for useQuery/useMutation without type filter
rg "useQuery|useMutation" --countRepository: CatholicOS/ontokit-web
Length of output: 301
🏁 Script executed:
# Check if React Query is imported anywhere
rg "from.*react-query" --countRepository: CatholicOS/ontokit-web
Length of output: 336
🏁 Script executed:
# Read one of the React Query hooks to see the pattern
cat -n lib/hooks/useEntityHistory.tsRepository: CatholicOS/ontokit-web
Length of output: 721
🏁 Script executed:
# Also check useCrossReferences.ts for another example
cat -n lib/hooks/useCrossReferences.ts | head -80Repository: CatholicOS/ontokit-web
Length of output: 686
Migrate this server-state fetch to React Query.
This hook manually implements server-state management with useEffect + useState and cancellation logic, but the repo pattern is to use useQuery for all server state. See useEntityHistory.ts and useCrossReferences.ts for the established pattern: a single useQuery call with appropriate queryKey, queryFn, enabled condition, and staleTime. This will consolidate loading/error handling, provide automatic cache/dedup, and align with the coding guideline: "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/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.
Comment on lines
+67
to
+101
| if (!graphData || expandedNodes.current.has(iri)) return; | ||
| expandedNodes.current.add(iri); | ||
|
|
||
| // Resolve labels for non-class entities | ||
| if (failedIrisRef.current.size > 0) { | ||
| await resolveNonClassLabels(); | ||
| } | ||
| } | ||
|
|
||
| buildGraph(focusIri); | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| graphApi | ||
| .getEntityGraph(projectId, iri, { | ||
| branch, | ||
| ancestorsDepth: 1, | ||
| descendantsDepth: 1, | ||
| maxNodes: 50, | ||
| }) | ||
| .then((newData) => { | ||
| setGraphData((prev) => { | ||
| if (!prev) return newData; | ||
|
|
||
| const existingNodeIds = new Set(prev.nodes.map((n) => n.id)); | ||
| const existingEdgeIds = new Set(prev.edges.map((e) => e.id)); | ||
|
|
||
| return { | ||
| ...prev, | ||
| nodes: [ | ||
| ...prev.nodes, | ||
| ...newData.nodes.filter((n) => !existingNodeIds.has(n.id)), | ||
| ], | ||
| edges: [ | ||
| ...prev.edges, | ||
| ...newData.edges.filter((e) => !existingEdgeIds.has(e.id)), | ||
| ], | ||
| total_concept_count: | ||
| prev.total_concept_count + newData.nodes.filter((n) => !existingNodeIds.has(n.id)).length, | ||
| }; | ||
| }); | ||
| }) | ||
| .catch(() => { | ||
| // Silently fail expansion | ||
| }); |
There was a problem hiding this comment.
Don't commit an expansion before the fetch is accepted.
expandedNodes.current.add(iri) happens before the request resolves. That makes transient failures non-retriable, and the in-flight response can still merge into a different focus graph or after resetGraph() because it is never scoped to the graph instance it was started from. Track "expanding" separately and only promote a node to expanded after the response is applied to 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 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).
Comment on lines
+84
to
+96
| return { | ||
| ...prev, | ||
| nodes: [ | ||
| ...prev.nodes, | ||
| ...newData.nodes.filter((n) => !existingNodeIds.has(n.id)), | ||
| ], | ||
| edges: [ | ||
| ...prev.edges, | ||
| ...newData.edges.filter((e) => !existingEdgeIds.has(e.id)), | ||
| ], | ||
| total_concept_count: | ||
| prev.total_concept_count + newData.nodes.filter((n) => !existingNodeIds.has(n.id)).length, | ||
| }; |
There was a problem hiding this comment.
Preserve truncation metadata when merging expansion results.
The merged object keeps prev.truncated, so an expansion response that hit maxNodes will still surface as truncated: false and the warning badge in OntologyGraph never appears. This should at least OR the flag with newData.truncated.
🧩 Minimal merge fix
return {
...prev,
+ truncated: prev.truncated || newData.truncated,
nodes: [
...prev.nodes,
...newData.nodes.filter((n) => !existingNodeIds.has(n.id)),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return { | |
| ...prev, | |
| nodes: [ | |
| ...prev.nodes, | |
| ...newData.nodes.filter((n) => !existingNodeIds.has(n.id)), | |
| ], | |
| edges: [ | |
| ...prev.edges, | |
| ...newData.edges.filter((e) => !existingEdgeIds.has(e.id)), | |
| ], | |
| total_concept_count: | |
| prev.total_concept_count + newData.nodes.filter((n) => !existingNodeIds.has(n.id)).length, | |
| }; | |
| return { | |
| ...prev, | |
| truncated: prev.truncated || newData.truncated, | |
| nodes: [ | |
| ...prev.nodes, | |
| ...newData.nodes.filter((n) => !existingNodeIds.has(n.id)), | |
| ], | |
| edges: [ | |
| ...prev.edges, | |
| ...newData.edges.filter((e) => !existingEdgeIds.has(e.id)), | |
| ], | |
| total_concept_count: | |
| prev.total_concept_count + newData.nodes.filter((n) => !existingNodeIds.has(n.id)).length, | |
| }; |
🤖 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 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.
Comment on lines
106
to
109
| const resetGraph = useCallback(() => { | ||
| resolvedNodesRef.current = new Map(); | ||
| failedIrisRef.current = new Set(); | ||
| nonClassLabelsRef.current = new Map(); | ||
| expandedNodes.current = new Set(); | ||
| setGraphData(null); | ||
| setResolvedCount(0); | ||
| }, []); |
There was a problem hiding this comment.
resetGraph() never restores the baseline graph.
This only clears local state. Because none of the fetch-effect dependencies change, the initial BFS request is never re-run, so the toolbar reset leaves the hook empty until some other prop or toggle 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 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.
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
components/graph/OntologyNode.tsx
Outdated
| aria-label={`${label}${nodeType === "unexplored" ? " (click to expand)" : ""}`} | ||
| > | ||
| <Handle type="target" position={Position.Bottom} className="bg-slate-400! w-2! h-2! border-0!" /> | ||
| <Handle type="target" position={Position.Top} className="bg-slate-400! w-2! h-2! border-0!" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether LR direction is propagated to node rendering and whether side handles are used.
set -euo pipefail
echo "1) Inspect current handle positions in OntologyNode:"
rg -n --type=tsx 'Handle\s+type="(target|source)".*position=\{Position\.(Top|Bottom|Left|Right)\}' components/graph/OntologyNode.tsx -C2
echo
echo "2) Find direction toggles / ELK direction sources:"
rg -n --type=ts --type=tsx 'layoutDirection|direction|elk.*(TB|LR)|Top-Down|Left-Right' components lib app -g '!**/*.test.*' -C2
echo
echo "3) Check if any graph node data already carries layout direction:"
rg -n --type=ts --type=tsx 'nodeType.*GraphNodeType|OntologyNodeData|layoutDirection' components lib app -g '!**/*.test.*' -C2Repository: CatholicOS/ontokit-web
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Verify handle positions and direction support in graph components
set -euo pipefail
echo "=== 1) Check OntologyNode.tsx for Handle positions (lines 95, 133) ==="
if [ -f "components/graph/OntologyNode.tsx" ]; then
cat -n components/graph/OntologyNode.tsx | sed -n '85,105p'
echo ""
cat -n components/graph/OntologyNode.tsx | sed -n '125,140p'
else
echo "File not found"
fi
echo ""
echo "=== 2) Search for layoutDirection usage in codebase ==="
rg -n 'layoutDirection|direction.*LR|direction.*TB' components lib app --type ts -g '!**/*.test.*' 2>/dev/null || true
echo ""
echo "=== 3) Check OntologyNodeData interface definition ==="
rg -n 'interface OntologyNodeData' components lib --type ts -A 10 2>/dev/null || true
echo ""
echo "=== 4) Search for direction toggle / layout direction in graph setup ==="
rg -n 'TB|LR|Top-Down|Left-Right|layoutDirection|direction' components/graph lib/graph app --type ts 2>/dev/null | head -30 || true
echo ""
echo "=== 5) Check OntologyEdge for position/direction handling ==="
if [ -f "components/graph/OntologyEdge.tsx" ]; then
cat -n components/graph/OntologyEdge.tsx | head -100
fiRepository: CatholicOS/ontokit-web
Length of output: 7437
🏁 Script executed:
#!/bin/bash
# Final verification: Check how nodes are created and passed to React Flow
echo "=== Check how OntologyGraph creates/renders nodes ==="
if [ -f "components/graph/OntologyGraph.tsx" ]; then
# Look for where nodes are created/mapped
rg -n 'nodes.*=|setNodes|createElement.*Node' components/graph/OntologyGraph.tsx -B2 -A5 | head -50
fi
echo ""
echo "=== Check if layoutDirection is ever passed to nodes ==="
rg -n 'layoutDirection.*node|node.*layoutDirection' components lib --type ts 2>/dev/null || echo "No matches found"Repository: CatholicOS/ontokit-web
Length of output: 1981
Hardcoded handle positions (Top/Bottom) cause incorrect edge routing in Left-Right layout mode.
The direction state in OntologyGraph properly toggles between "TB" and "LR" and is passed to ELK layout, but node handle positions remain fixed to Position.Top and Position.Bottom regardless of layout direction. When the graph switches to LR mode, ELK correctly lays out nodes horizontally, but edges still attempt to route from top/bottom anchors, resulting in poor visual quality and incorrect routing.
The node data must include the current layout direction so that handles can adapt. Modify the ELK layout hook to include layoutDirection in the node data, update OntologyNodeData interface to accept it, and conditionally set handle positions:
Proposed fix
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!"
+ />Also update the ELK layout hook to pass direction to node data when creating nodes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/graph/OntologyNode.tsx` at 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.
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) |
Port entity graph from folio-mapper to use server-driven BFS endpoint. Replace buildGraphData.ts and elkLayout.ts with useELKLayout hook and lib/graph/utils.ts. Add EntityGraphModal for full-screen graph view. Update OntologyGraph, OntologyNode, OntologyEdge for server data model. Remove accessToken and labelHints props from layout components. - New: lib/api/graph.ts (API client for graph endpoint) - New: lib/graph/useELKLayout.ts (ELK layout hook from folio-mapper) - New: lib/graph/utils.ts (extractTreeLabelMap utility) - New: components/graph/EntityGraphModal.tsx (full-screen overlay) - Rewritten: useGraphData.ts (single API call + progressive expansion) - Updated: OntologyGraph, OntologyNode, OntologyEdge - Updated: DeveloperEditorLayout, StandardEditorLayout Ref: #81 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Include CONTEXT.md, HANDOFF.md, and PLAN.md from the entity graph port planning session for reviewer context. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
Comment on lines
+23
to
+27
| ### Backend graph endpoint | ||
| - **D-04:** Add a server-side BFS graph building endpoint to ontokit-api, ported from folio-mapper's `build_entity_graph()` in `folio_service.py` | ||
| - **D-05:** Endpoint signature: `GET /concept/{iri_hash}/graph?ancestors_depth=5&descendants_depth=2&max_nodes=200&include_see_also=true` | ||
| - **D-06:** Returns `EntityGraphResponse` with nodes, edges, truncation flag, and total concept count | ||
| - **D-07:** Full stack in one feature — backend endpoint + frontend components ship together |
There was a problem hiding this comment.
Update this API contract to the project-scoped endpoint used by the PR.
This section still documents the older /concept/{iri_hash}/graph shape, but this feature now uses the project-scoped /api/v1/projects/{id}/graph/entity-graph contract via graphApi.getEntityGraph(projectId, focusIri, ...). Leaving the old contract here will send follow-up work—and the downstream PLAN examples that copy it—in the wrong direction.
🤖 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 - 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.
Comment on lines
+76
to
+78
| ``` | ||
| GET /ontologies/{ontology_id}/classes/{class_iri:path}/graph | ||
| ``` |
There was a problem hiding this comment.
Add languages to these fenced blocks.
Both fences still trip markdownlint MD040. text is enough here since they are literal examples, not executable snippets.
Minimal fix
-```
+```text
GET /ontologies/{ontology_id}/classes/{class_iri:path}/graph
-```
+```
-```
+```text
Plan 1 (backend) → Plan 2 (frontend components) → Plan 3 (integration)
-```
+```Also applies to: 238-240
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 76-76: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 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.
Comment on lines
+34
to
36
| if (!focusIri) { | ||
| setGraphData(null); | ||
| return; |
There was a problem hiding this comment.
Reset isLoading when the focus is cleared.
If focusIri becomes null while a request is in flight, this early return skips setIsLoading(false) and the cancelled request's finally skips it too. The hook can stay stuck in a loading state until another fetch starts.
Minimal fix
if (!focusIri) {
+ setIsLoading(false);
setGraphData(null);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!focusIri) { | |
| setGraphData(null); | |
| return; | |
| 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, 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/api/v1/projects/{id}/graph/entity-graph) — 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 navigateWhat changed
buildGraphData.tsparsed tree structuregraphApi.getEntityGraph()calls backend BFSelkLayout.tsstatic wrapperuseELKLayouthook with dynamic widthsEntityGraphModaloverlaybuildGraphData.test.tsuseELKLayout.test.ts,utils.test.ts,useGraphData.test.tsFiles changed (16 files, 2 commits)
New:
lib/api/graph.ts— API client for server-side BFS endpointlib/graph/useELKLayout.ts— ELK layered layout hook with dynamic node widthslib/graph/utils.ts—extractTreeLabelMap()utilitycomponents/graph/EntityGraphModal.tsx— full-screen graph overlayRewritten:
lib/hooks/useGraphData.ts— single API call + progressiveexpandNode()components/graph/OntologyGraph.tsx— server data model, toolbar, descendants toggleUpdated:
components/graph/OntologyNode.tsx— root nodes red (was amber)components/graph/OntologyEdge.tsx— seeAlso purple dashedcomponents/editor/developer/DeveloperEditorLayout.tsx— import path, removed old propscomponents/editor/standard/StandardEditorLayout.tsx— expand button + EntityGraphModalDeleted:
lib/graph/buildGraphData.ts— replaced by server-side BFSlib/graph/elkLayout.ts— replaced byuseELKLayouthook__tests__/lib/graph/buildGraphData.test.ts— replaced by new testsPlanning context:
.planning/features/entity-graph-port/— CONTEXT, HANDOFF, PLANVerification
catholicos/main(a1f496d)Test plan
Requires backend: CatholicOS/ontokit-api#37
Closes #81
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests