fix(library): guard read-only assessment popovers

Author: FelixTJDietrich

library/lib/hooks/useElementInteractions.ts

@@ -1,5 +1,5 @@
 import { usePopoverStore } from "@/store/context"
-import { useMetadataStore } from "@/store"
+import { useDiagramStore, useMetadataStore } from "@/store"
 import { ApollonMode } from "@/typings"
 import {
   NodeMouseHandler,
@@ -12,10 +12,19 @@ import { useShallow } from "zustand/shallow"
 import { useDiagramModifiable } from "./useDiagramModifiable"
 import { isElementInOverlay } from "@/keyboard"
 import { useCallback } from "react"
+import { hasAssessmentToShow } from "@/utils/assessmentPresence"
 
 export const useElementInteractions = () => {
   const isDiagramModifiable = useDiagramModifiable()
-  const mode = useMetadataStore((state) => state.mode)
+  const { mode, readonly } = useMetadataStore(
+    useShallow((state) => ({ mode: state.mode, readonly: state.readonly }))
+  )
+  const { nodes, getAssessment } = useDiagramStore(
+    useShallow((state) => ({
+      nodes: state.nodes,
+      getAssessment: state.getAssessment,
+    }))
+  )
   const { setPopOverElementId } = usePopoverStore(
     useShallow((state) => ({
       setPopOverElementId: state.setPopOverElementId,
@@ -59,18 +68,38 @@ export const useElementInteractions = () => {
 
   const onNodeClick: NodeMouseHandler<Node> = useCallback(
     (_event, node) => {
-      if (!canOpenAssessmentPopover) return
+      if (
+        !canOpenAssessmentPopover ||
+        (readonly && !hasAssessmentToShow(node.id, nodes, getAssessment))
+      )
+        return
       setPopOverElementId(node.id)
     },
-    [canOpenAssessmentPopover, setPopOverElementId]
+    [
+      canOpenAssessmentPopover,
+      getAssessment,
+      nodes,
+      readonly,
+      setPopOverElementId,
+    ]
   )
 
   const onEdgeClick: EdgeMouseHandler<Edge> = useCallback(
     (_event, edge) => {
-      if (!canOpenAssessmentPopover) return
+      if (
+        !canOpenAssessmentPopover ||
+        (readonly && !hasAssessmentToShow(edge.id, nodes, getAssessment))
+      )
+        return
       setPopOverElementId(edge.id)
     },
-    [canOpenAssessmentPopover, setPopOverElementId]
+    [
+      canOpenAssessmentPopover,
+      getAssessment,
+      nodes,
+      readonly,
+      setPopOverElementId,
+    ]
   )
   return {
     onBeforeDelete,

library/tests/unit/elementInteractionsAssessment.test.tsx

@@ -0,0 +1,56 @@
+import { beforeEach, describe, expect, it, vi } from "vitest"
+import { act, renderHook } from "@testing-library/react"
+import { ApollonMode } from "@/typings"
+
+const setPopOverElementId = vi.fn()
+const nodes = [{ id: "node", data: {} }]
+const assessments: Record<string, { score?: number; feedback?: string }> = {}
+
+vi.mock("@/store", () => ({
+  useMetadataStore: (select: (state: unknown) => unknown) =>
+    select({ mode: ApollonMode.Assessment, readonly: true }),
+  useDiagramStore: (select: (state: unknown) => unknown) =>
+    select({
+      nodes,
+      getAssessment: (id: string) => assessments[id],
+    }),
+}))
+
+vi.mock("@/store/context", () => ({
+  usePopoverStore: (select: (state: unknown) => unknown) =>
+    select({ setPopOverElementId }),
+}))
+
+vi.mock("@/hooks/useDiagramModifiable", () => ({
+  useDiagramModifiable: () => false,
+}))
+
+import { useElementInteractions } from "@/hooks/useElementInteractions"
+
+describe("read-only assessment element interactions", () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    for (const id of Object.keys(assessments)) delete assessments[id]
+  })
+
+  it("opens only nodes and edges that have feedback to show", () => {
+    const { result } = renderHook(() => useElementInteractions())
+    const node = { id: "node" }
+    const edge = { id: "edge" }
+
+    act(() => {
+      result.current.onNodeClick(undefined as never, node as never)
+      result.current.onEdgeClick(undefined as never, edge as never)
+    })
+    expect(setPopOverElementId).not.toHaveBeenCalled()
+
+    assessments.node = { score: 1 }
+    assessments.edge = { feedback: "Useful feedback" }
+    act(() => {
+      result.current.onNodeClick(undefined as never, node as never)
+      result.current.onEdgeClick(undefined as never, edge as never)
+    })
+    expect(setPopOverElementId).toHaveBeenNthCalledWith(1, "node")
+    expect(setPopOverElementId).toHaveBeenNthCalledWith(2, "edge")
+  })
+})

library/tests/unit/onBeforeDelete.test.tsx

@@ -3,22 +3,33 @@ import type { ReactNode } from "react"
 import { renderHook } from "@testing-library/react"
 import { createMetadataStore } from "@/store/metadataStore"
 import { createPopoverStore } from "@/store/popoverStore"
-import { MetadataStoreContext, PopoverStoreContext } from "@/store/context"
+import { createDiagramStore } from "@/store/diagramStore"
+import {
+  DiagramStoreContext,
+  MetadataStoreContext,
+  PopoverStoreContext,
+} from "@/store/context"
 import { useElementInteractions } from "@/hooks/useElementInteractions"
+import * as Y from "yjs"
 
 /**
  * `onBeforeDelete` is the one gate every React Flow deletion funnels through.
  * Besides read-only, it blocks a Delete pressed while focus is in an overlay
  * over the canvas — React Flow's delete listener is document-level, so without
  * this a dialog's Delete would remove the selection behind it.
  */
-const wrapper = ({ children }: { children: ReactNode }) => (
-  <MetadataStoreContext value={createMetadataStore()}>
-    <PopoverStoreContext value={createPopoverStore()}>
-      {children}
-    </PopoverStoreContext>
-  </MetadataStoreContext>
-)
+const wrapper = ({ children }: { children: ReactNode }) => {
+  const ydoc = new Y.Doc()
+  return (
+    <DiagramStoreContext value={createDiagramStore(ydoc)}>
+      <MetadataStoreContext value={createMetadataStore(ydoc)}>
+        <PopoverStoreContext value={createPopoverStore()}>
+          {children}
+        </PopoverStoreContext>
+      </MetadataStoreContext>
+    </DiagramStoreContext>
+  )
+}
 
 const onBeforeDelete = () =>
   renderHook(() => useElementInteractions(), { wrapper }).result.current
@@ -34,10 +45,13 @@ describe("useElementInteractions.onBeforeDelete", () => {
   it("keeps React Flow callback identities stable across parent renders", () => {
     const metadata = createMetadataStore()
     const popover = createPopoverStore()
+    const diagram = createDiagramStore(new Y.Doc())
     const stableWrapper = ({ children }: { children: ReactNode }) => (
-      <MetadataStoreContext value={metadata}>
-        <PopoverStoreContext value={popover}>{children}</PopoverStoreContext>
-      </MetadataStoreContext>
+      <DiagramStoreContext value={diagram}>
+        <MetadataStoreContext value={metadata}>
+          <PopoverStoreContext value={popover}>{children}</PopoverStoreContext>
+        </MetadataStoreContext>
+      </DiagramStoreContext>
     )
     const hook = renderHook(() => useElementInteractions(), {
       wrapper: stableWrapper,

library/tests/unit/revealAssessment.test.ts

@@ -208,6 +208,8 @@ describe("ApollonEditor.revealAssessment", () => {
       model: MODEL,
     })
 
+    const selectionChanges: string[][] = []
+    editor.subscribeToSelectionChange((ids) => selectionChanges.push(ids))
     editor.revealAssessment("edge-ab")
 
     const internals = editor as unknown as {
@@ -222,6 +224,7 @@ describe("ApollonEditor.revealAssessment", () => {
     expect(internals.diagramStore.getState().selectedElementIds).toEqual([
       "edge-ab",
     ])
+    expect(selectionChanges).toEqual([["edge-ab"]])
     expect(getEdgesMap(internals.ydoc).get("edge-ab")?.selected).toBeUndefined()
     expect(internals.diagramStore.getState().undoManager?.undoStack).toEqual([])
   })