fix(graph): address high-severity findings from PR #88 review

[damienriehl]

Summary

Follow-up to #88 addressing the 3 high-severity findings from peer review. Targets entity-graph-pr so fixes land before #88 merges to dev.

Recommended merge order: this PR (#XX) → #88 → dev.

Findings addressed

Sev	ID	Issue	Commit
High	H1	OntologyNode advertised itself as a focusable button (role="button" tabIndex={0}) with onKeyDown Enter/Space handlers, but useELKLayout never populated onNavigate / onExpandNode props — so screen readers announced an interactive element that did nothing.	ad95639
High	H2	graphEdgeStyle Zustand store field was added, persisted to localStorage, defaulted to 'smoothstep', AND exposed via a toolbar — but OntologyEdge always called getBezierPath. The setting silently lied to the user.	df83589
High	H3	OntologyGraphNode / OntologyGraphEdge / GraphData were leftover from the deleted client-side graph builder. After the server-side BFS port, EntityGraphResponse is canonical and grep confirms no consumers reference the leftovers.	724ea45

Detailed rationale per fix

H1 — Wire keyboard handlers through

The challenge: ReactFlow already owns mouse interactions via onNodeClick / onNodeDoubleClick at the OntologyGraph level. If OntologyNode adds its own onClick, mouse events fire BOTH handlers (expand AND navigate). Solution:

1. Store handlers in a ref inside useELKLayout so swapping them does not re-run layout or bust React.memo on OntologyNode. Stable lambdas in node data read from the ref at event time.
2. useELKLayout exports setNodeHandlers({ onNavigate, onExpandNode }) and the consumer calls it from a useEffect.
3. Drop competing onClick / onDoubleClick from OntologyNode — let ReactFlow own all mouse events.
4. Keep onKeyDown for keyboard parity with the new mapping:
   • Enter / Space → expand (matches single-click)
   • Shift + Enter → navigate (matches double-click)
5. aria-label describes the keyboard shortcuts so screen readers announce both actions.

// useELKLayout.ts — handlers in ref, stable lambdas in node data
const handlersRef = useRef<NodeHandlers>({});
const stableHandlers = useMemo(
  () => ({
    onNavigate: (iri: string) => handlersRef.current.onNavigate?.(iri),
    onExpandNode: (iri: string) => handlersRef.current.onExpandNode?.(iri),
  }),
  [],
);
const setNodeHandlers = useCallback((h: NodeHandlers) => {
  handlersRef.current = h;
}, []);

// OntologyGraph.tsx — feed handlers without re-laying out
useEffect(() => {
  setNodeHandlers({ onNavigate: onNavigateToClass, onExpandNode: expandNode });
}, [setNodeHandlers, onNavigateToClass, expandNode]);

H2 — Honor graphEdgeStyle setting

// OntologyEdge.tsx
const edgeStyle = useEditorModeStore((s) => s.graphEdgeStyle);
const pathBuilder = edgeStyle === "smoothstep" ? getSmoothStepPath : getBezierPath;
const [edgePath, labelX, labelY] = pathBuilder({ ... });

OntologyEdge.test.tsx mocks the store (default 'smoothstep') and the new getSmoothStepPath import to keep tests fast and matchMedia-free.

H3 — Drop redundant types

OntologyGraphNode / OntologyGraphEdge / GraphData were render-side mirrors of the API types but no consumer imported them after the BFS port. Removed; only the GraphNodeType / GraphEdgeType literal unions remain (rendering-side mirrors of the Python Literal[...] in ontokit-api ontokit/schemas/graph.py from the companion fix in #37 follow-up).

Verification

• All 2,516 existing tests pass — none regressed
• npm run type-check clean
• npm run lint — 21 warnings (all pre-existing in unrelated files; no new ones)

npm test -- --run             # 2516 passed (was 2507; +9 retests after API change)
npm run type-check            # clean

Test plan

• npm test -- --run __tests__/components/graph/OntologyNode.test.tsx — keyboard contract verified (Enter expands, Shift+Enter navigates, external nodes don't dispatch)
• npm test -- --run __tests__/components/graph/OntologyEdge.test.tsx — smoothstep path used (per default store mock)
• In browser: focus a node with Tab, press Enter → graph expands; press Shift+Enter → navigates to class detail
• Settings → toggle Graph edge style → bezier vs smoothstep visibly differs

Findings deferred to Discussion threads

The original review also flagged 5 medium and 5 low items. They will be filed as a tracked Discussion on this repo for follow-up.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ac8df814-c337-4921-a228-d0c5163c0a31

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/pr-88-blockers

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

❌ Patch coverage is 86.09865% with 31 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...omponents/editor/standard/StandardEditorLayout.tsx	20.00%	7 Missing and 1 partial ⚠️
components/graph/OntologyGraph.tsx	84.90%	5 Missing and 3 partials ⚠️
lib/graph/useELKLayout.ts	87.50%	3 Missing and 3 partials ⚠️
components/graph/EntityGraphModal.tsx	85.71%	1 Missing and 4 partials ⚠️
lib/hooks/useGraphData.ts	96.15%	1 Missing and 1 partial ⚠️
components/graph/OntologyEdge.tsx	66.66%	0 Missing and 1 partial ⚠️
lib/stores/editorModeStore.ts	0.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[damienriehl]

Closing as superseded by #88 (entity-graph-pr).

After PR #228 (back-to-project) merged to dev, I rebased entity-graph-pr against the new dev tip. All H1/H2/H3 fixes that were originally the point of this PR are already on the rebased entity-graph-pr (commits 1fadb47, 059ac12, e2a0c70 = original H3, H2, H1 in renamed form), along with the rest of the entity-graph evolution.

The one piece of this PR that wasn't already in #88 — Fr. John's b42496e edge style toggle UI in the graph toolbar — has been ported directly onto #88 in commit 02cec20 (with positive test coverage). Co-author credit preserved on the commit.

#88 is now the single authoritative branch for this work. Safe to close.