e2e: rewrite 19 Konva fixme scenarios to use data-annotator-shapes model (#1616)

[steilerDev]

Summary

Implements the E2E test rewrite for issue #1616.

PhotoAnnotator.tsx now exposes undoStack.shapes as data-annotator-shapes JSON on the [role="application"] container. All 19 test.fixme scenarios that were blocked by Konva canvas rendering (no SVG DOM nodes) are rewritten as active test() calls.

Changes

Part A — e2e/pages/PhotoViewerPage.ts

• Updated header doc-comment: removed stale SVG element type list, replaced with Konva/JSON attribute description
• Added shape type declarations: local copies of all AnnotationShape variants from useUndoStack.ts (RectangleShape, HighlightShape, ArrowShape, LineShape, EllipseShape, TextShape, MeasurementShape, FreehandShape, AnnotationShape) — field names verified against source
• Replaced shapeByType() and committedShapeCount() with getAnnotatorShapes() which reads data-annotator-shapes JSON from [role="application"]

Part B — e2e/tests/photoAnnotation.spec.ts

• All 19 test.fixme removed, converted to active test()
• SVG DOM locators replaced with expect.poll(() => viewer.getAnnotatorShapes(), { timeout: 15_000 }) assertions
• Scenarios 2, 3, 22 (passing) untouched
• All API response assertions, tool aria-pressed checks, inline input interactions, test.setTimeout() calls preserved

Part C Checklist

• grep -c "TODO: rewrite for Konva canvas" e2e/tests/photoAnnotation.spec.ts → 0
• All 19 scenarios use test( not test.fixme(
• getAnnotatorShapes() exists in POM
• shapeByType() and committedShapeCount() removed from POM
• All expect.poll() calls use { timeout: 15_000 }
• Zero new TypeScript errors in photoAnnotation.spec.ts
• Scenarios 2, 3, 22 untouched
• Shape type field names verified against useUndoStack.ts

Closes #1616

[steilerDev]

[ux-designer]

UX & Accessibility Review — data-annotator-shapes attribute

This is a tech-debt fix with one production-code change: adding data-annotator-shapes={JSON.stringify(undoStack.shapes)} to the [role="application"] canvas container. My analysis covers the four questions raised.

---

1. Does data-annotator-shapes affect role="application" ARIA semantics?

No impact. data-* attributes are entirely opaque to the ARIA accessibility tree — the spec explicitly reserves them for custom application data and they carry no implicit ARIA role, property, or state. The role="application" and aria-label={t("canvas")} semantics remain exactly as intended: a landmark that signals to screen readers that keyboard interaction is handled by the application, not the browser.

---

2. Could screen readers announce data-annotator-shapes unexpectedly?

No. Screen readers (NVDA, JAWS, VoiceOver, TalkBack) do not read data-* attributes unless a component explicitly references them via aria-describedby or similar. The attribute is invisible to the accessibility tree.

One small note: role="application" suppresses most virtual-cursor reading of descendant content, which is correct here since Konva renders to a <canvas> element — there is no meaningful DOM text for a screen reader to traverse anyway. The aria-live="polite" live region (per the Style Guide Photo Annotator pattern) remains the correct and sole channel for communicating shape operations to screen reader users.

---

3. Visual regression risk?

None. The attribute has no CSS side-effects — no attribute selectors in the annotator stylesheets target data-annotator-shapes. The style={{ position: "relative" }} and className={styles.canvasArea} lines are unchanged. There is zero rendering, layout, or paint difference between before and after this change.

---

4. Privacy / UX concern with serializing shape data to the DOM?

Low risk; worth a brief note for the team.

The serialized payload includes:

• Geometric coordinates (x, y, w, h, points, etc.) — image-space, not GPS or real-world data.
• User-typed strings: TextShape.text (free text annotations) and MeasurementShape.label (e.g. "3.5m") — these could contain sensitive site notes.
• Stroke colors (fixed palette values, not PII).

Key mitigations:

• The data is transient: it exists only for the duration of the annotation session and disappears when the annotator closes.
• It is not a new exposure: anything in undoStack.shapes is already committed to the server on Save. The DOM attribute does not surface data that was not already travelling over the wire.
• The primary consumer is Playwright e2e tests reading the attribute programmatically — a well-scoped purpose analogous to data-testid.

Non-blocking suggestion for a future refinement: if the team wants a tighter surface area, the text and label string fields could be stripped from the serialized payload (most e2e geometry assertions only need type plus numeric coordinates). Not required for this PR.

---

Review Checklist

Item	Result
Token adherence	No CSS changes
Dark mode correctness	Unaffected
Visual regression	None — purely a DOM attribute
ARIA / accessibility tree	data-* is opaque to AT
Screen reader announcement	No unexpected announcements possible
Keyboard navigation	Unaffected
Shared component reuse	N/A — no new UI elements
prefers-reduced-motion	N/A

Verdict: No UX or accessibility objections. Safe to merge.

[steilerDev]

[product-architect]

Architecture Review — PR #1625: fix/1616-konva-e2e-fixme

I've reviewed the full diff, the useUndoStack.ts source, useAnnotator.ts, PhotoAnnotator.tsx, and the ADR-011 E2E architecture contract.

---

Verdict: ✅ Approve

The approach is architecturally sound. All five key questions from the brief have clear answers. Minor findings noted below — none are blockers.

---

Q1 — Is undoStack.shapes the correct source of truth for committed shapes?

Yes — definitively correct.

undoStack.shapes is stack.present from useUndoStack, which is always the most recently committed snapshot. It is never the draft. useAnnotator.ts line 212 (state: { ...state, shapes: undoStack.shapes }) also re-projects undoStack.shapes back into the reducer state on every render, confirming they stay in lock-step. The draft lives only in state.draftShape (a separate nullable field). Exposing undoStack.shapes therefore correctly excludes in-progress drawing strokes and reflects only fully committed shapes — exactly what the tests need to assert.

---

Q2 — Is data-annotator-shapes JSON on [role="application"] architecturally sound?

Yes, and it is the right choice for this problem.

The three alternatives are:

Option	Assessment
Konva stage serialization (.toJSON())	Couples tests to Konva's internal node tree. Brittle across Konva version bumps and subject to canvas-specific properties (scale, filters) that have no test relevance.
Visual regression (screenshot diff)	Completely impractical for geometric property assertions like rx === ry, y1 ≈ y2, or text === 'Inspection point'.
data-annotator-shapes JSON	Exposes exactly the application's domain model. Zero coupling to rendering internals. Reactive via React's normal DOM commit cycle.

The attribute is added to an element the E2E POM already locates (svgOverlay = page.locator('[role="application"]')), so no new selector surface is created. The approach aligns with the ADR-011 principle that tests exercise the production image as a black box — the attribute is part of the production DOM.

One nuance worth documenting: this attribute is test instrumentation living in production code. This is acceptable at Cornerstone's scale (<5 users, single container) and the payload is small. A future ADR could formalize the convention of data-* test-bridge attributes if the pattern is reused.

---

Q3 — Performance: JSON.stringify(undoStack.shapes) on every render — acceptable?

Yes, with a note.

For <5 users and a bounded annotation canvas (tens of shapes, not thousands), the serialization overhead is negligible — measured in microseconds per render. React's reconciler will diff the resulting string and skip the DOM write if the value is unchanged (same reference equality from useState). Since commit() always produces a new array reference ([...prev.past, prev.present]), the string will correctly update on every commit.

The only edge case to be aware of: during live drag (mouse-move events), state.draftShape changes but undoStack.shapes does NOT (the draft is not committed yet). This means the attribute value is stable during dragging, which is precisely what you want — no high-frequency serialization during gesture handling.

---

Q4 — Local type declarations in PhotoViewerPage.ts vs. shared package?

Correct decision to keep them local. No action needed.

The @cornerstone/shared package (shared/src/types/) does not export AnnotationShape or any shape variant — these types are currently client-internal. ADR-011 explicitly states e2e/ has "no dependency on @cornerstone/server or @cornerstone/client" and "tests the built Docker image as a black box."

Importing directly from client/src/ would violate this boundary and couple the e2e workspace to the client build. The local copies in PhotoViewerPage.ts are the right pattern, with one request: add a sync-check comment near the local declarations (e.g., // SOURCE: client/src/components/photos/PhotoAnnotator/useUndoStack.ts — keep in sync) so future maintainers know exactly where to look when the canonical types change. The PR description mentions "field names verified against source" but that context lives only in the PR, not in the code.

---

Q5 — AnnotationShape interface field names vs. useUndoStack.ts — do they match?

Yes — all 8 shape variants match exactly.

I verified each field name against useUndoStack.ts:

Shape	POM Fields	Source Fields	Match
RectangleShape	x, y, w, h, color, strokeWidth	identical	✅
HighlightShape	x, y, w, h, color	identical (no strokeWidth)	✅
ArrowShape	x1, y1, x2, y2, stroke, strokeWidth	identical	✅
LineShape	x1, y1, x2, y2, stroke, strokeWidth	identical	✅
EllipseShape	cx, cy, rx, ry, stroke, strokeWidth	identical	✅
TextShape	x, y, text, fontSize, color	identical	✅
MeasurementShape	x1, y1, x2, y2, label, stroke, strokeWidth, fontSize, color	identical	✅
FreehandShape	points: [number, number][], stroke, strokeWidth	identical	✅

---

Additional Findings

Medium — Double-read after expect.poll (minor brittleness risk)

In several scenarios (5, 6, 7, 9, 15, 19, 23), getAnnotatorShapes() is called a second time immediately after expect.poll resolves:

await expect.poll(async () => {
  const shapes = await viewer.getAnnotatorShapes();
  return shapes.some(s => s.type === 'arrow');
}, { timeout: 15_000 }).toBe(true);

// Second call:
const shapes5 = await viewer.getAnnotatorShapes();
const arrow = shapes5.find(s => s.type === 'arrow') as ArrowShape | undefined;

In practice this is safe because React's DOM commit is synchronous — once poll sees the attribute, it won't disappear in the next async tick. However, the redundant round-trip adds one extra getAttribute IPC call per scenario. The cleaner pattern is to capture the shapes inside the final poll iteration and return them directly, or use the last polled value. Not a blocker; suggested improvement for follow-up.

Medium — Optional if (arrow) guards after expect(arrow).toBeDefined()

In scenarios 5, 6, 7, 9, 15, and 19, the pattern is:

expect(line7).toBeDefined();
if (line7) {
  expect(Math.abs(line7.y1 - line7.y2)).toBeLessThan(2);
}

The if guard is defensive but redundant — if toBeDefined() fails, the test aborts. TypeScript's control-flow analysis doesn't narrow undefined away after expect().toBeDefined() (it's a runtime Playwright assertion, not a TS type guard), so the if is needed to satisfy the type checker. This is therefore correct and intentional. No action needed — just noting it is not a bug.

Low — HighlightShape missing from the doc-comment shape-type list

The POM header comment (line 43–46) lists: rectangle, highlight, arrow, line, ellipse, text, measurement, freehand. highlight is present ✅. No issue.

Low — callout removed cleanly

The PR correctly removes callout from the type union (it was already deleted from the tool palette per the spec file comment at line 1123). The doc comment header listing (8 types) is consistent with useUndoStack.ts. ✅

Informational — No ADR update required

This change does not introduce a new architectural pattern that rises to ADR level. The data-* attribute test-bridge convention is a localized extension of ADR-011's black-box testing principle. If this pattern is adopted in a second component, an ADR should be written then.

---

Summary

Question	Finding	Severity
undoStack.shapes as source of truth	Correct	—
data-annotator-shapes attribute approach	Sound and recommended	—
JSON.stringify on every render	Acceptable at this scale	—
Local type declarations in e2e/	Correct; add a sync-check comment	Low
Field name accuracy	All 8 shapes match exactly	—
Double-read after poll	Redundant but harmless; suggest refactor	Medium
Optional if guards	Correct TypeScript pattern	—

One low action item: add a // SOURCE: ... comment near the local shape type declarations so maintainers know where to find the canonical types when useUndoStack.ts evolves.

LGTM otherwise. 🟢

[steilerDev]

[frontend-developer] ✅ Frontend review complete.

Production code change reviewed:

• File: client/src/components/photos/PhotoAnnotator/PhotoAnnotator.tsx (line ~902)
• Change: Adds data-annotator-shapes={JSON.stringify(undoStack.shapes)} to the application container <div>

Assessment:

1. Performance: ✅ Acceptable

   • JSON.stringify(undoStack.shapes) runs on every render, but this is safe: the number of shapes is bounded by user interaction (typically <50 shapes in a photo annotation session). The serialization cost is negligible compared to rendering the Konva canvas itself. The attribute updates reactively as expected (on commit/undo/redo/clear).

2. React patterns: ✅ No anti-patterns

   • The string identity changes on each render (as intended — it reflects the current state). This does not cause render thrashing because React attributes are not dependency array values; the string is inert data read by e2e tests via getAttribute(), not used in React event handlers or useEffect dependencies.

3. Test-only attributes: ✅ Convention consistent

   • Using data-* for test instrumentation is well-established throughout the codebase (data-testid, data-status, data-gantt-sidebar-row, data-row-id, data-theme, data-entity-type, etc.). This attribute follows the pattern and has minimal production impact.

4. No other client/ files touched: ✅ Verified

   • Only PhotoAnnotator.tsx is modified; e2e tests are out of scope for this review.

Verdict: ✅ Approved. The implementation is clean, performant, and consistent with existing conventions.

[steilerDev]

[dev-team-lead]

Code Review — PR #1625: e2e: rewrite 19 Konva fixme scenarios

VERDICT: CHANGES_REQUIRED — one blocking lint issue, one low-severity item carried from the architect review.

---

Issue 1: Unused AnnotationShape type import — ESLint error (BLOCKING)

• File: e2e/tests/photoAnnotation.spec.ts
• Line: 55
• Problem: AnnotationShape is included in the import type { ... } statement but is never referenced in the test body. With @typescript-eslint/no-unused-vars: error configured in eslint.config.js (global rule, no e2e override), this will fail linting in CI.
• Fix: Remove AnnotationShape from the import line:

  import type { RectangleShape, EllipseShape, ArrowShape, LineShape, FreehandShape, TextShape, MeasurementShape } from '../pages/PhotoViewerPage.js';

• Agent: e2e-test-engineer

---

Issue 2: Missing // SOURCE: sync-check comment on local shape type declarations (LOW — carrying from architect review)

• File: e2e/pages/PhotoViewerPage.ts
• Line: ~50 (above // ── Annotator shape types ... block)
• Problem: The product-architect requested a // SOURCE: comment near the local type declarations so maintainers know exactly where to find the canonical definitions when useUndoStack.ts evolves. Without it, the comment in the PR description is the only clue.
• Fix: Add a single comment line, e.g.:

  // ── Annotator shape types (mirror of client/src/.../useUndoStack.ts) ─────────
  // SOURCE: client/src/components/photos/PhotoAnnotator/useUndoStack.ts — keep in sync
  // Local copies so e2e/ does not import from client/ source.

• Agent: e2e-test-engineer

---

Everything else: APPROVED ✅

• All 19 previously-skipped test.fixme scenarios are now active (zero fixme remaining).
• data-annotator-shapes attribute correctly placed on [role="application"] div, sourced from undoStack.shapes (committed state only, not draft).
• getAnnotatorShapes() correctly reads data-annotator-shapes from the svgOverlay locator.
• All 8 shape interface field names in PhotoViewerPage.ts match useUndoStack.ts exactly.
• expect.poll(getAnnotatorShapes()) pattern is used consistently — no trivial stubs.
• All geometry assertions are substantive (Shift-snap y1≈y2, circle snap rx≈ry, color field, move delta, etc.).
• ESM .js extensions present on all imports; import type used correctly for shape types.
• shapes[0]?.type uses optional chaining (correct for noUncheckedIndexedAccess).
• The if (shape) guards after expect(shape).toBeDefined() are correct TypeScript pattern (TS doesn't narrow after runtime Playwright assertions).