diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 0fac53db1..4c536b4e8 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -33,10 +33,18 @@ jobs: working-directory: packages/types run: pnpm build + - name: Build artifact-canvas package (+ build-smoke) + working-directory: packages/artifact-canvas + run: pnpm build && pnpm build:smoke + - name: Run core unit tests working-directory: packages/core run: pnpm test + - name: Run artifact-canvas unit tests + working-directory: packages/artifact-canvas + run: pnpm test + - name: Copy skeleton for unit tests working-directory: packages/codev run: pnpm copy-skeleton diff --git a/.gitignore b/.gitignore index 900735c1e..f9a8afb02 100644 --- a/.gitignore +++ b/.gitignore @@ -31,6 +31,7 @@ packages/codev/dist/ packages/codev/skeleton/ packages/types/dist/ packages/core/dist/ +packages/artifact-canvas/dist/ packages/vscode/dist/ packages/vscode/out/ *.vsix diff --git a/codev/plans/945-build-foundational-reusable-pa.md b/codev/plans/945-build-foundational-reusable-pa.md new file mode 100644 index 000000000..1d87c865a --- /dev/null +++ b/codev/plans/945-build-foundational-reusable-pa.md @@ -0,0 +1,428 @@ +--- +approved: 2026-06-10 +approval_note: >- + Approved at the SPIR plan-approval gate (status.yaml: approved_at 2026-06-10). 5 plan consult + iterations — Claude APPROVE x5; Codex REQUEST_CHANGES x5 (progressively smaller build/test/release + wiring items, the last two self-inflicted by revisions); Gemini lane unavailable (agy). Per the + human decision recorded in the Consultation Log below, the final two Codex items were fixed and the + plan taken to the gate rather than a 6th consult round. +validated: [claude] # plan iter-5 APPROVE; codex REQUEST_CHANGES resolved pre-gate; gemini lane unavailable +--- + +# Plan: Foundational reusable package `@cluesmith/codev-artifact-canvas` + +## Metadata +- **ID**: plan-2026-06-09-945-build-foundational-reusable-pa +- **Status**: approved +- **Specification**: [codev/specs/945-build-foundational-reusable-pa.md](../specs/945-build-foundational-reusable-pa.md) +- **GitHub Issue**: [#945](https://github.com/cluesmith/codev/issues/945) +- **Created**: 2026-06-09 + +## Executive Summary + +Build the shared library `@cluesmith/codev-artifact-canvas` (Approach A in the spec: one +React package + per-host adapter seams). The work splits into four committable phases: +(1) package skeleton + dual-format build + locked interfaces + theme tokens; (2) the +markdown renderer with `data-line` mapping + D7 sanitization; (3) the comment overlay +(intent-only) + v1 marker rendering + adapter wire-up + auto-refresh; (4) the smoke-test +host + README + cross-cutting tests. No host integration ships here (that's #859 / the +dashboard route / mobile). + +This plan also **resolves the five items deferred from the spec consult** (the plan-gate +acceptance criteria) — see the **Deferred-Item Resolutions** section, which maps each to the +phase that closes it. + +## Locked plan-level decisions (closing spec Open Questions §3/§4) + +- **Build tool = `tsup`** (closes spec Open Q §3). It emits CJS + ESM + `.d.ts` from one + config with minimal setup, handles TSX, and can bundle/copy the stylesheet. Vite library + mode and raw esbuild were the alternatives; tsup is the lightest path to the spec's required + dual-format output. The build-smoke test (a CJS `require()` + an ESM `import()`) guards it. +- **`default-theme.css` ships as a separate export path** (closes spec Open Q §4): + `@cluesmith/codev-artifact-canvas/default-theme.css`. Explicit, host-overridable, not + auto-injected — hosts opt in via ``/import and override the `--codev-canvas-*` vars. + +## Success Metrics +- [ ] All spec acceptance criteria met (functional + non-functional). +- [ ] All 5 deferred items resolved or consciously decided (see Deferred-Item Resolutions). +- [ ] Package source has zero `vscode` / `node:*` / direct `fs`/`fetch` imports (import-boundary test green). +- [ ] Dual CJS+ESM bundle + `.d.ts` builds; build-smoke test green. +- [ ] New package `test` script green and wired into the monorepo build graph. +- [ ] No regression to #857 (editor-side review flow untouched). + +## Phases (Machine Readable) + +```json +{ + "phases": [ + {"id": "phase_1", "title": "Package skeleton, dual-format build, locked interfaces, theme tokens"}, + {"id": "phase_2", "title": "Markdown renderer: data-line mapping + DOMPurify sanitization"}, + {"id": "phase_3", "title": "Comment overlay (intent-only) + v1 marker rendering + adapter wire-up"}, + {"id": "phase_4", "title": "Smoke-test host, README, cross-cutting tests"} + ] +} +``` + +## Deferred-Item Resolutions (plan-gate acceptance criteria) + +| # | Source | Item | Resolution | Phase | +|---|---|---|---|---| +| 1 | Codex | D2 "injectable logger" claim has no matching prop | **Drop the injectability claim.** Internal diagnostics go to `console`; the host-facing hook is the existing `onError?(err)` prop in `ArtifactCanvasProps`. No logger prop added. Spec D2 text adjusted accordingly during this phase's doc pass. | P1 (contract) | +| 2 | Codex | `ThemeAdapter.resolve` token format ambiguous | **Pin to the full custom-property name** — `resolve("--codev-canvas-foreground")`, 1:1 with the D4 vocabulary, no hidden bare-name mapping. Documented on the interface + README. | P1 (contract) | +| 3 | Codex | Sanitization test doesn't exercise DOMPurify (`html:false` neutralizes ``, a `javascript:` URL) renders +with that content neutralized — no script executes and no event-handler attribute survives. + +## Solution Approaches (alternatives considered) + +### Approach A — Shared React package + per-host adapters *(chosen)* +Build the renderer, overlay, and adapter seams once; hosts implement three adapters. +- **Pros:** one renderer/overlay to maintain; UX parity across surfaces by construction; + makes #859 thin and the dashboard route + mobile cheap; the adapter seam is the natural + test boundary. +- **Cons:** introduces the repo's first React-component-library package and first dual-format + build; the contract must be right up front (mitigated by this spec + the smoke-test host). +- **Complexity:** Medium. **Risk:** Medium (contract lock-in) — addressed by SPIR's gates. + +### Approach B — Framework-agnostic Web Components *(rejected)* +- **Pros:** host-framework-neutral; embeddable anywhere. +- **Cons:** throws away the dashboard's React investment; React↔custom-element interop and + styling/theming friction; the team's component idioms are React. The issue explicitly + rejects this. + +### Approach C — Build per surface, no shared package *(rejected — the status quo trap)* +- **Pros:** each surface optimal in isolation; no new package. +- **Cons:** three renderers + three overlays to maintain; UX divergence; every #858–#863 + feature implemented up to three times. This is exactly what the issue exists to prevent. + +### Approach D — Extend VSCode's built-in markdown preview *(rejected — infeasible)* +#859 already established the built-in preview's contribution points are render-only with no +host back-channel, so comment-from-preview is impossible without owning the surface. + +## Open Questions + +### Critical (blocks progress) — none +All decisions needed to begin are resolved above. + +### Important (affects design) + +1. **REVIEW marker format reconciliation.** The issue body states the marker form is + `` and calls it "the existing convention from #857". + **Codebase verification shows that is not the current convention** — #857 writes positional + `` (line implied by file position; regex captures author + + text only). **Proposed resolution (per D3):** the package stays serialization-agnostic; the + in-memory `ReviewMarker` carries `line` (derived from position on read) and `raw` (for + round-tripping). The VSCode host preserves the positional #857 form (satisfying the + "no regression" AC); a host that wants explicit `line=N`/`lines=N-M` may opt in without the + package mandating it. *This will be raised with the architect at the spec-approval gate so + the "existing convention" wording can be confirmed or corrected.* + +2. **Smoke-test host form.** Issue leaves it to the implementer: a Vite dev-server route or a + minimal VSCode webview. **Proposed:** a Vite dev-server harness inside the package + (`examples/`), since it exercises the ESM build and the React components without VSCode + tooling, runs in CI headlessly, and doubles as living adapter-implementation documentation. + +### Nice-to-know (optimization) + +3. **Build tool for the dual bundle** (`tsup` vs Vite library mode vs raw esbuild). A plan + decision; the spec only requires the CJS+ESM+types+CSS output. +4. **Whether `default-theme.css` ships as a separate import path** (`.../default-theme.css`) + vs auto-injected. Leaning separate import (explicit, tree-shakeable, host-overridable). + +## Success Criteria / Acceptance Criteria + +Functional (MUST): +- [ ] `packages/artifact-canvas/` exists; `package.json` declares + `@cluesmith/codev-artifact-canvas`, peer-deps on `react`/`react-dom`, deps on + `markdown-it` and `dompurify`. +- [ ] Renderer produces HTML with `data-line` attributes on block tokens (paragraphs, + headings, list items, code blocks, blockquotes, tables); a unit test covers the + attribution. +- [ ] A comment-overlay component renders a hover-`+` on rendered blocks; clicking invokes the + **`onAddComment(line: number)`** prop (the canonical intent seam, `ArtifactCanvasProps`); + the package does **not** call `MarkerAdapter.add` (text-input + write-back are host-owned, + D6). A unit test asserts the intent-prop contract. +- [ ] The hover-`+` affordance is **keyboard-accessible** — reachable via keyboard focus and + activatable with Enter/Space (not hover-only), with an accessible label for screen + readers; a test covers keyboard activation. (iter-2 Claude) +- [ ] The public API exports the three adapter interfaces (`FileAdapter`, `MarkerAdapter`, + `ThemeAdapter`), the data types `ReviewMarker` and `Disposable`, and the component props + `ArtifactCanvasProps`; the package has zero direct filesystem, `fetch`, or VSCode-API + imports (import-boundary test). +- [ ] Theming via CSS custom properties; the package ships a default stylesheet defining a + fallback for **each v1 `--codev-canvas-*` token (D4)**; hosts override by setting the + variables; documented host override examples. `ThemeAdapter.resolve()` is JS-side only + (D4, Model A) and is not on the v1 render path. +- [ ] A smoke-test host demonstrates end-to-end: load sample markdown → render → hover → + click `+` → adapter receives the call → marker round-trips. +- [ ] Build produces a **CJS + ESM** bundle (with type declarations) consumable by both a + VSCode webview and the dashboard's Vite/ESM pipeline. +- [ ] **Text-as-source-of-truth invariant test**: no affordance produces output that isn't + either a source-markdown text mutation or a clearly delimited adjacent text artifact. + (For v1 concretely: the comment overlay's only output channel is the `onAddComment` + intent event — no side-channel writes.) +- [ ] **HTML-sanitization (D7)**: markdown-it runs with `html: false` and output is + DOMPurify-sanitized before render; rendered HTML contains **no executable script + content** even when the input markdown attempts to embed it (` + + diff --git a/packages/artifact-canvas/examples/main.tsx b/packages/artifact-canvas/examples/main.tsx new file mode 100644 index 000000000..d9fbc8ce8 --- /dev/null +++ b/packages/artifact-canvas/examples/main.tsx @@ -0,0 +1,44 @@ +/** + * Vite dev page for hands-on/visual exercise of (Phase 4 — a developer aid, + * NOT the contract proof; the automated proof is `src/__tests__/end-to-end.test.tsx`). + * + * Launch with `pnpm dev:example` (= `vite examples`). It reuses the SAME stub adapters + sample + * artifact as the e2e test, so what you click here is exactly what the test asserts: hover a + * block, click the `+` (or focus + Enter), type a comment, and watch it round-trip through text + * back into the rendered markers. Excluded from the published package (`files`/`exports`). + */ +import * as React from 'react'; +import { createRoot } from 'react-dom/client'; +import { ArtifactCanvas } from '../src/components/ArtifactCanvas.js'; +import { createStubHost } from '../src/__tests__/fixtures/stub-adapters.js'; +import { SAMPLE_ARTIFACT } from '../src/__tests__/fixtures/sample-artifact.js'; +import '../src/styles/default-theme.css'; + +const host = createStubHost(SAMPLE_ARTIFACT); + +function Example(): React.ReactElement { + const onAddComment = (line: number) => { + // Host glue (spec D6): the package emits intent; the host collects input + writes back. + const text = window.prompt(`Comment on line ${line + 1}:`); + if (text) void host.markerAdapter.add('artifact://sample.md', line, text, 'you'); + }; + return React.createElement( + 'div', + { style: { maxWidth: 760, margin: '2rem auto', padding: '0 1rem' } }, + React.createElement('h2', null, 'artifact-canvas dev example'), + React.createElement( + 'p', + { style: { color: '#6e7781' } }, + 'Hover a block for the + affordance (or focus it and press Enter). Comments round-trip through text.', + ), + React.createElement(ArtifactCanvas, { + uri: 'artifact://sample.md', + fileAdapter: host.fileAdapter, + markerAdapter: host.markerAdapter, + themeAdapter: host.themeAdapter, + onAddComment, + }), + ); +} + +createRoot(document.getElementById('root')!).render(React.createElement(Example)); diff --git a/packages/artifact-canvas/examples/vite.config.ts b/packages/artifact-canvas/examples/vite.config.ts new file mode 100644 index 000000000..54ae9136e --- /dev/null +++ b/packages/artifact-canvas/examples/vite.config.ts @@ -0,0 +1,10 @@ +import { defineConfig } from 'vite'; + +/** + * Dev-only config for the `examples/` page (`pnpm dev:example`). Kept minimal — no React plugin + * dependency; esbuild's automatic JSX runtime transpiles the .tsx (HMR full-reload is fine for a + * smoke page). The example is never built into the published package. + */ +export default defineConfig({ + esbuild: { jsx: 'automatic' }, +}); diff --git a/packages/artifact-canvas/package.json b/packages/artifact-canvas/package.json new file mode 100644 index 000000000..f19a29d85 --- /dev/null +++ b/packages/artifact-canvas/package.json @@ -0,0 +1,52 @@ +{ + "name": "@cluesmith/codev-artifact-canvas", + "version": "3.1.9", + "description": "Reusable React surface for rendering and reviewing Codev markdown artifacts (specs/plans/reviews) across VSCode, the dashboard, and future mobile hosts.", + "type": "module", + "exports": { + ".": { + "types": "./dist/index.d.ts", + "import": "./dist/index.js", + "require": "./dist/index.cjs" + }, + "./default-theme.css": "./dist/default-theme.css" + }, + "main": "./dist/index.cjs", + "module": "./dist/index.js", + "types": "./dist/index.d.ts", + "files": [ + "dist" + ], + "scripts": { + "build": "tsup", + "test": "vitest run", + "build:smoke": "node scripts/smoke.mjs", + "check-types": "tsc --noEmit", + "dev:example": "vite examples" + }, + "peerDependencies": { + "react": "^18 || ^19", + "react-dom": "^18 || ^19" + }, + "dependencies": { + "dompurify": "^3.2.0", + "markdown-it": "^14.1.0" + }, + "devDependencies": { + "@testing-library/jest-dom": "^6.6.0", + "@testing-library/react": "^16.0.0", + "@types/dompurify": "^3.0.5", + "@types/markdown-it": "^14.1.2", + "@types/node": "^22.0.0", + "@types/react": "^19.0.0", + "@types/react-dom": "^19.0.0", + "jsdom": "^26.0.0", + "react": "^19.0.0", + "react-dom": "^19.0.0", + "tsup": "^8.3.0", + "typescript": "^5.7.0", + "vite": "^6.0.0", + "vitest": "^4.0.0" + }, + "license": "Apache-2.0" +} diff --git a/packages/artifact-canvas/scripts/smoke.mjs b/packages/artifact-canvas/scripts/smoke.mjs new file mode 100644 index 000000000..e7c53ef05 --- /dev/null +++ b/packages/artifact-canvas/scripts/smoke.mjs @@ -0,0 +1,25 @@ +/** + * Build-smoke check (spec test scenario 7): after `pnpm build`, verify the dual-format output + * actually loads — the CJS entry via require() and the ESM entry via import(). Run with: + * node scripts/smoke.mjs + * (intended to run after `pnpm build`, e.g. as a CI step). + */ +import { createRequire } from 'node:module'; +import { existsSync } from 'node:fs'; + +const require = createRequire(import.meta.url); +const fail = (msg) => { console.error('build-smoke FAIL:', msg); process.exit(1); }; + +for (const f of ['dist/index.cjs', 'dist/index.js', 'dist/index.d.ts', 'dist/default-theme.css']) { + if (!existsSync(f)) fail(`missing build artifact: ${f}`); +} + +// CJS require() +const cjs = require('../dist/index.cjs'); +if (typeof cjs.ArtifactCanvas !== 'function') fail('CJS entry missing ArtifactCanvas export'); + +// ESM import() +const esm = await import('../dist/index.js'); +if (typeof esm.ArtifactCanvas !== 'function') fail('ESM entry missing ArtifactCanvas export'); + +console.log('build-smoke OK: CJS + ESM entries load; ArtifactCanvas exported; dist assets present.'); diff --git a/packages/artifact-canvas/src/__tests__/end-to-end.test.tsx b/packages/artifact-canvas/src/__tests__/end-to-end.test.tsx new file mode 100644 index 000000000..e25d668a7 --- /dev/null +++ b/packages/artifact-canvas/src/__tests__/end-to-end.test.tsx @@ -0,0 +1,104 @@ +/** + * End-to-end contract proof (Phase 4 — the PRIMARY deliverable). + * + * Mounts the real against the stub host adapters + the sample artifact and + * proves the full review round-trip *through text* (spec D3 / text-as-source-of-truth, #857), + * via BOTH mouse and keyboard: + * + * render → existing marker shows → hover/focus a block → click/Enter the `+` → + * onAddComment(line) fires (0-based, spec D5) → host glue calls MarkerAdapter.add → + * the marker is serialized INTO the text → FileAdapter.watch replays the new text → + * MarkerAdapter.list re-derives from that text → the new marker renders. + * + * It asserts the new marker text lands in the shared text store (not an in-memory side store), + * which is what distinguishes a real round-trip from a mere UI refresh (iter-2 Codex). + */ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, fireEvent, waitFor, cleanup } from '@testing-library/react'; +import * as React from 'react'; +import { ArtifactCanvas } from '../components/ArtifactCanvas.js'; +import { SAMPLE_ARTIFACT } from './fixtures/sample-artifact.js'; +import { createStubHost } from './fixtures/stub-adapters.js'; + +afterEach(cleanup); + +/** Host glue (spec D6): the package emits intent; the host performs input + write-back. */ +function mountWithHost(initial: string, author = 'reviewer', body = 'please clarify') { + const host = createStubHost(initial); + const onAddComment = vi.fn((line: number) => { + void host.markerAdapter.add('artifact://sample.md', line, body, author); + }); + render( + , + ); + return { host, onAddComment }; +} + +async function firstParagraph(): Promise { + return waitFor(() => { + const el = document.querySelector('p[data-line]'); + if (!el) throw new Error('no paragraph rendered yet'); + return el as HTMLElement; + }); +} + +describe('ArtifactCanvas end-to-end (Phase 4 contract proof)', () => { + it('renders the sample artifact and its pre-existing marker', async () => { + mountWithHost(SAMPLE_ARTIFACT); + await waitFor(() => expect(document.querySelector('h1')).not.toBeNull()); + expect(document.body.textContent).toContain('Example Feature'); + // The fixture's seeded REVIEW marker derives from text and renders. + await waitFor(() => expect(document.querySelector('.codev-canvas-has-marker')).not.toBeNull()); + }); + + it('MOUSE: hover → click "+" → round-trips a new marker THROUGH TEXT', async () => { + const { host, onAddComment } = mountWithHost(SAMPLE_ARTIFACT, 'alice', 'tighten this'); + const p = await firstParagraph(); + const line = Number(p.getAttribute('data-line')); + + fireEvent.mouseOver(p); + fireEvent.click(await screen.findByRole('button', { name: /add comment on line/i })); + + // Intent fired with the 0-based line; the package never wrote the marker itself (D6). + expect(onAddComment).toHaveBeenCalledWith(line); + + // The host's add serialized the marker INTO the text store (proves round-trip through text). + await waitFor(() => + expect(host.store.getText()).toContain(''), + ); + // …and after watch → re-list, the new marker renders on the annotated line. + await waitFor(() => { + const marked = document.querySelectorAll('.codev-canvas-has-marker'); + const onLine = Array.from(marked).some( + (el) => Number(el.getAttribute('data-line')) === line, + ); + expect(onLine).toBe(true); + }); + }); + + it('KEYBOARD: focus → Enter → round-trips a new marker THROUGH TEXT', async () => { + const { host, onAddComment } = mountWithHost(SAMPLE_ARTIFACT, 'bob', 'needs a test'); + const p = await firstParagraph(); + const line = Number(p.getAttribute('data-line')); + + expect(p.tabIndex).toBe(0); // keyboard-reachable (accessibility AC) + fireEvent.keyDown(p, { key: 'Enter' }); + + expect(onAddComment).toHaveBeenCalledWith(line); + await waitFor(() => + expect(host.store.getText()).toContain(''), + ); + await waitFor(() => { + const onLine = Array.from(document.querySelectorAll('.codev-canvas-has-marker')).some( + (el) => Number(el.getAttribute('data-line')) === line, + ); + expect(onLine).toBe(true); + }); + }); +}); diff --git a/packages/artifact-canvas/src/__tests__/fixtures/sample-artifact.ts b/packages/artifact-canvas/src/__tests__/fixtures/sample-artifact.ts new file mode 100644 index 000000000..870109424 --- /dev/null +++ b/packages/artifact-canvas/src/__tests__/fixtures/sample-artifact.ts @@ -0,0 +1,28 @@ +/** + * A representative Codev review artifact (markdown) used by the end-to-end test and the + * `examples/` dev page. It exercises the renderer's block variety (headings, paragraph, list, + * fenced code) so the smoke surface looks like a real spec/review, and it already carries one + * positional `` marker so the "existing markers render" path is covered + * without any host write. + * + * Text is the source of truth (spec D3 / #857): the marker on the line below "## Summary" + * annotates the heading block ABOVE it. + */ +export const SAMPLE_ARTIFACT = `# Spec 42 — Example Feature + +## Summary + +This document describes an example feature so the canvas has realistic content to render. + +## Requirements + +- Render markdown safely. +- Surface review markers inline. +- Emit comment intent to the host. + +\`\`\`ts +export function example(): number { + return 42; +} +\`\`\` +`; diff --git a/packages/artifact-canvas/src/__tests__/fixtures/stub-adapters.ts b/packages/artifact-canvas/src/__tests__/fixtures/stub-adapters.ts new file mode 100644 index 000000000..db277fc0f --- /dev/null +++ b/packages/artifact-canvas/src/__tests__/fixtures/stub-adapters.ts @@ -0,0 +1,102 @@ +/** + * Stub host adapters for the end-to-end test and the `examples/` dev page (Phase 4). + * + * These are NOT shipped (they live under `src/__tests__/` and are excluded from `files`/`exports`). + * They implement the three package interfaces against a single shared **text store** so the + * round-trip is proven *through text* (spec D3 / text-as-source-of-truth, #857), not via an + * in-memory side store: + * + * - `stubMarkerAdapter.add` serializes a positional `` INTO the + * text (on the line *after* the annotated block, #857 convention) and notifies watchers. + * - `stubFileAdapter.read` returns the current text; `.watch` replays text changes. + * - `stubMarkerAdapter.list` DERIVES markers by parsing the current text. + * + * The store is created per call (`createStore`) so each test/page instance is isolated — the + * stubs are factories over a store rather than module-level singletons. + */ +import type { FileAdapter } from '../../adapters/FileAdapter.js'; +import type { MarkerAdapter } from '../../adapters/MarkerAdapter.js'; +import type { ThemeAdapter } from '../../adapters/ThemeAdapter.js'; +import type { Disposable, ReviewMarker } from '../../types.js'; + +/** Mutable text store shared by a host's three adapters — text is the single source of truth. */ +export interface StubStore { + getText(): string; + setText(next: string): void; + /** Active watch callbacks (exposed so tests/pages can assert teardown if needed). */ + watchers: Array<(content: string) => void>; +} + +export function createStore(initial: string): StubStore { + let text = initial; + const watchers: Array<(content: string) => void> = []; + return { + getText: () => text, + setText: (next: string) => { + text = next; + watchers.forEach((cb) => cb(text)); + }, + watchers, + }; +} + +const REVIEW_RE = //; + +/** Parse positional REVIEW comments out of the text into ReviewMarkers (text → markers). */ +function parseMarkers(text: string): ReviewMarker[] { + const out: ReviewMarker[] = []; + text.split('\n').forEach((ln, i) => { + const m = ln.match(REVIEW_RE); + // #857: a REVIEW comment sits on the line BELOW the block it annotates, so its logical + // (0-based) line is i-1. A comment on line 0 annotates nothing and is skipped. + if (m && i > 0) out.push({ author: m[1], line: i - 1, text: m[2], raw: ln.trim() }); + }); + return out; +} + +export function stubFileAdapter(store: StubStore): FileAdapter { + return { + read: async (_uri: string) => store.getText(), + watch: (_uri: string, onChange: (content: string) => void): Disposable => { + store.watchers.push(onChange); + return { + dispose: () => { + const i = store.watchers.indexOf(onChange); + if (i >= 0) store.watchers.splice(i, 1); // idempotent: a second call is a no-op + }, + }; + }, + }; +} + +export function stubMarkerAdapter(store: StubStore): MarkerAdapter { + return { + list: async (_uri: string) => parseMarkers(store.getText()), + add: async (_uri: string, line: number, text: string, author: string) => { + const lines = store.getText().split('\n'); + // Serialize INTO the text on the line after the annotated block (#857), then notify. + lines.splice(line + 1, 0, ``); + store.setText(lines.join('\n')); + }, + }; +} + +export function stubThemeAdapter(): ThemeAdapter { + // Off the v1 render path (spec D4 Model A) — provided only to satisfy the prop + the + // scenario-4 contract. resolve() returns a token's value; onChange registers a no-op handler. + return { + resolve: (token: string) => (token === '--codev-canvas-foreground' ? '#1f2328' : ''), + onChange: (_handler: () => void): Disposable => ({ dispose: () => {} }), + }; +} + +/** Convenience: one shared store wired to all three stub adapters (used by the e2e test + page). */ +export function createStubHost(initial: string) { + const store = createStore(initial); + return { + store, + fileAdapter: stubFileAdapter(store), + markerAdapter: stubMarkerAdapter(store), + themeAdapter: stubThemeAdapter(), + }; +} diff --git a/packages/artifact-canvas/src/__tests__/import-boundary.test.ts b/packages/artifact-canvas/src/__tests__/import-boundary.test.ts new file mode 100644 index 000000000..c3bd1dfe4 --- /dev/null +++ b/packages/artifact-canvas/src/__tests__/import-boundary.test.ts @@ -0,0 +1,53 @@ +import { describe, it, expect } from 'vitest'; +import { readFileSync, readdirSync, statSync } from 'node:fs'; +import { join, dirname } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +/** + * Import-boundary guard (spec: the package has zero direct filesystem / fetch / VSCode-API + * imports). All I/O flows through host-supplied adapters. Test files themselves are excluded + * (they legitimately use node:fs to scan the tree). + */ + +const here = dirname(fileURLToPath(import.meta.url)); +const srcRoot = join(here, '..'); + +function collectSourceFiles(dir: string): string[] { + const out: string[] = []; + for (const entry of readdirSync(dir)) { + const full = join(dir, entry); + if (statSync(full).isDirectory()) { + if (entry === '__tests__' || entry === 'node_modules') continue; + out.push(...collectSourceFiles(full)); + } else if (/\.(ts|tsx)$/.test(entry) && !/\.test\.(ts|tsx)$/.test(entry)) { + out.push(full); + } + } + return out; +} + +const FORBIDDEN: Array<{ label: string; pattern: RegExp }> = [ + { label: 'vscode', pattern: /from\s+['"]vscode['"]|require\(\s*['"]vscode['"]\s*\)/ }, + { label: 'node:* builtin', pattern: /from\s+['"]node:[^'"]+['"]|require\(\s*['"]node:[^'"]+['"]\s*\)/ }, + { label: "bare 'fs'", pattern: /from\s+['"]fs['"]|require\(\s*['"]fs['"]\s*\)/ }, + { label: 'fetch()', pattern: /\bfetch\s*\(/ }, +]; + +describe('import boundary', () => { + const files = collectSourceFiles(srcRoot); + + it('finds source files to scan', () => { + expect(files.length).toBeGreaterThan(0); + }); + + it('shipped source contains no vscode / node:* / fs / fetch usage', () => { + const violations: string[] = []; + for (const file of files) { + const text = readFileSync(file, 'utf8'); + for (const { label, pattern } of FORBIDDEN) { + if (pattern.test(text)) violations.push(`${file}: ${label}`); + } + } + expect(violations).toEqual([]); + }); +}); diff --git a/packages/artifact-canvas/src/adapters/FileAdapter.ts b/packages/artifact-canvas/src/adapters/FileAdapter.ts new file mode 100644 index 000000000..8f2805a85 --- /dev/null +++ b/packages/artifact-canvas/src/adapters/FileAdapter.ts @@ -0,0 +1,22 @@ +import type { Disposable } from '../types.js'; + +/** + * Reads document content and notifies on external change. + * + * Async/sync split (spec D2): `read` is async (Promise-returning); `watch` is **synchronous** + * — it registers a subscription and returns a `Disposable` immediately, while change + * notifications arrive asynchronously via the supplied callback. + * + * This is an interface only — implementations live in the host (spec: adapters carry no + * implementations in the package). + */ +export interface FileAdapter { + /** Read the current document content for `uri`. */ + read(uri: string): Promise; + /** + * Subscribe to external changes to `uri`. Returns a `Disposable` synchronously; `onChange` + * fires (asynchronously) with the new content. The host is responsible for any + * debouncing/coalescing before invoking `onChange` (spec D2). + */ + watch(uri: string, onChange: (content: string) => void): Disposable; +} diff --git a/packages/artifact-canvas/src/adapters/MarkerAdapter.ts b/packages/artifact-canvas/src/adapters/MarkerAdapter.ts new file mode 100644 index 000000000..d24ba7eeb --- /dev/null +++ b/packages/artifact-canvas/src/adapters/MarkerAdapter.ts @@ -0,0 +1,24 @@ +import type { ReviewMarker } from '../types.js'; + +/** + * Reads and mutates review markers. Serialization is the implementation's concern (spec D3). + * + * In v1 the package calls only `list`; `add` is invoked by host glue code (spec D6 — the + * overlay emits an `onAddComment` intent and the host performs the text input + write-back). + * + * Interface only — implementations live in the host. + */ +export interface MarkerAdapter { + /** List the review markers currently present in `uri`. */ + list(uri: string): Promise; + /** + * Write a new review marker. Host-invoked (spec D6); the package never calls this itself in + * v1. Serialization (positional `` for the VSCode host, or any + * other text form) is the implementation's choice. + */ + add(uri: string, line: number, text: string, author: string): Promise; + + // Reserved for later issues (declared optional so hosts may implement incrementally): + // addRegion?(uri: string, lineStart: number, lineEnd: number, text: string, author: string): Promise; + // setCheckbox?(uri: string, line: number, checked: boolean): Promise; // AC-progress (#862) +} diff --git a/packages/artifact-canvas/src/adapters/ThemeAdapter.ts b/packages/artifact-canvas/src/adapters/ThemeAdapter.ts new file mode 100644 index 000000000..2a4082e73 --- /dev/null +++ b/packages/artifact-canvas/src/adapters/ThemeAdapter.ts @@ -0,0 +1,22 @@ +import type { Disposable } from '../types.js'; + +/** + * JS-side theme access for canvas-drawing consumers (spec D4, Model A). + * + * **NOT used by the v1 render path.** v1 theming is entirely CSS-custom-property driven: the + * component's styles bind to `--codev-canvas-*` variables and the host overrides those. This + * adapter exists for JS-side consumers that must read an exact value — chiefly #863's `` + * minimap, which has to read a hex color to paint pixels. In v1 it is exercised only by a + * standalone contract test, never by the canvas component. + * + * Interface only — implementations live in the host. + */ +export interface ThemeAdapter { + /** + * Resolve the current value of a theme token. `token` is the full custom-property name, + * e.g. `resolve("--codev-canvas-foreground")` (spec D4 — no bare-name mapping). + */ + resolve(token: string): string; + /** Register a handler fired (synchronously registered) when the host theme changes. */ + onChange(handler: () => void): Disposable; +} diff --git a/packages/artifact-canvas/src/components/ArtifactCanvas.tsx b/packages/artifact-canvas/src/components/ArtifactCanvas.tsx new file mode 100644 index 000000000..3f8f1fc90 --- /dev/null +++ b/packages/artifact-canvas/src/components/ArtifactCanvas.tsx @@ -0,0 +1,210 @@ +import * as React from 'react'; +import type { ArtifactCanvasProps, ReviewMarker } from '../types.js'; +import { renderMarkdown } from '../renderer/renderer.js'; +import { CommentAffordance } from '../overlays/CommentAffordance.js'; + +/** + * ArtifactCanvas — the composed review surface (Phase 3). + * + * Data flow (spec D2/D6): reads content via `FileAdapter.read`, lists markers via + * `MarkerAdapter.list`, and subscribes to `FileAdapter.watch` (the only subscription). When the + * file changes it re-renders and auto re-lists markers. It emits comment *intent* via + * `onAddComment(line)` and never calls `MarkerAdapter.add` itself (the host does the input + + * write-back). `themeAdapter` is accepted but NOT used for rendering (spec D4, Model A — theming + * is CSS-variable driven; `resolve()`/`onChange` are for #863's canvas). + * + * Errors from the adapter calls the component makes are caught, logged, and surfaced via the + * optional `onError` prop; the component never throws out of an event handler, and a failed + * `list()` leaves the prior markers in place (spec D2). + */ +export function ArtifactCanvas(props: ArtifactCanvasProps): React.ReactElement { + const { uri, fileAdapter, markerAdapter, onAddComment, onError, refreshKey } = props; + + const [content, setContent] = React.useState(''); + const [markers, setMarkers] = React.useState([]); + const [activeLine, setActiveLine] = React.useState(null); + const bodyRef = React.useRef(null); + + const report = React.useCallback( + (err: unknown) => { + console.error('[artifact-canvas]', err); + onError?.(err); + }, + [onError], + ); + + // Request-versioning: out-of-order async resolutions must never apply stale state — a slow + // initial read() or an older list() must not overwrite a newer watch update (iter-2 Codex). + // Each load (initial read or a watch change) takes a monotonically increasing seq; results are + // applied only while their seq is still the latest. + const seqRef = React.useRef(0); + // Warn-once dedup for out-of-range stale markers, so a noisy watch doesn't spam warnings + // across reloads (deferred #4 AC: "dropped … and warned once"). iter-3 Codex. + const warnedRef = React.useRef(new Set()); + + // Apply content + markers for one load, guarded by `seq`. Out-of-range markers are dropped + + // warned (deferred #4: ignore, not clamp/hard-error); a failed list() keeps the prior markers. + const applyLoad = React.useCallback( + async (text: string, seq: number) => { + if (seq !== seqRef.current) return; // superseded by a newer load + setContent(text); + try { + const list = await markerAdapter.list(uri); + if (seq !== seqRef.current) return; // a newer load won the race — discard these markers + const lineCount = text.length === 0 ? 0 : text.split('\n').length; + setMarkers( + list.filter((m) => { + const ok = m.line >= 0 && m.line < lineCount; + if (!ok) { + const key = `${m.line}|${m.author}|${m.text}`; + if (!warnedRef.current.has(key)) { + warnedRef.current.add(key); + console.warn( + `[artifact-canvas] dropping out-of-range marker @line ${m.line} (document has ${lineCount} lines)`, + ); + } + } + return ok; + }), + ); + } catch (err) { + if (seq !== seqRef.current) return; + report(err); // keep prior markers on failure + } + }, + [markerAdapter, uri, report], + ); + + // Initial read + the single watch subscription (spec D2/D6). + React.useEffect(() => { + let disposed = false; + const initialSeq = (seqRef.current += 1); + void (async () => { + try { + const text = await fileAdapter.read(uri); + if (disposed) return; + await applyLoad(text, initialSeq); + } catch (err) { + if (!disposed) report(err); + } + })(); + + let sub: { dispose(): void } = { dispose: () => {} }; + try { + sub = fileAdapter.watch(uri, (newContent) => { + if (disposed) return; + const watchSeq = (seqRef.current += 1); // each change is a newer load + void applyLoad(newContent, watchSeq); // auto re-list on change (D6) + }); + } catch (err) { + // A synchronous failure setting up the subscription must not throw out of the effect (D2). + report(err); + } + + return () => { + disposed = true; + sub.dispose(); // idempotent per the Disposable contract (spec D2) + }; + // `refreshKey` in the deps: a host without a watcher bumps it to force a fresh read+list (D6). + }, [fileAdapter, uri, applyLoad, report, refreshKey]); + + const html = React.useMemo(() => renderMarkdown(content), [content]); + + // Decorate the rendered (innerHTML) DOM after each render: make blocks keyboard-focusable and + // mark lines that carry a ReviewMarker (v1 minimal marker rendering — deferred #4; #863 adds + // polished inline markers + the canvas minimap). + React.useEffect(() => { + const root = bodyRef.current; + if (!root) return; + const byLine = new Map(); + for (const m of markers) { + const arr = byLine.get(m.line) ?? []; + arr.push(m); + byLine.set(m.line, arr); + } + // (tabindex is stamped at render time by the renderer, not here — keeps focusability free of + // this effect's timing.) This effect only applies marker decoration, which depends on the + // asynchronously-loaded markers. + root.querySelectorAll('[data-line]').forEach((el) => { + const line = Number(el.getAttribute('data-line')); + const ms = byLine.get(line); + if (ms && ms.length > 0) { + el.classList.add('codev-canvas-has-marker'); + el.setAttribute('title', ms.map((m) => `${m.author}: ${m.text}`).join('\n')); + } else { + el.classList.remove('codev-canvas-has-marker'); + el.removeAttribute('title'); + } + }); + // Reconcile the overlay anchor against the *reloaded* DOM (iter-5 Codex): if a watch/refreshKey + // reload removed or shortened the previously active block, clear `activeLine` so the overlay + // can't render `+` for — or emit `onAddComment` for — a line the new content no longer has. + // VALIDATE rather than blindly reset: a still-present active line survives, so this never races + // a fresh hover (which changes only `activeLine`, not `html`, so this effect doesn't run then). + setActiveLine((cur) => + cur !== null && !root.querySelector(`[data-line="${cur}"]`) ? null : cur, + ); + }, [html, markers]); + + const lineFromEvent = (target: EventTarget | null): number | null => { + const el = (target as HTMLElement | null)?.closest?.('[data-line]') as HTMLElement | null; + if (!el) return null; + const n = Number(el.getAttribute('data-line')); + return Number.isNaN(n) ? null : n; + }; + + // Markers on the currently-active (hovered/focused) line — surfaced author + text via the + // overlay (deferred #4: minimal v1 marker rendering; #863 adds polished inline markers). + const activeMarkers = activeLine === null ? [] : markers.filter((m) => m.line === activeLine); + + return React.createElement( + 'div', + { className: 'codev-artifact-canvas', onMouseLeave: () => setActiveLine(null) }, + React.createElement('div', { + ref: bodyRef, + className: 'codev-artifact-canvas-body', + onMouseOver: (e: React.MouseEvent) => { + const l = lineFromEvent(e.target); + if (l !== null) setActiveLine(l); + }, + onFocus: (e: React.FocusEvent) => { + const l = lineFromEvent(e.target); + if (l !== null) setActiveLine(l); + }, + onKeyDown: (e: React.KeyboardEvent) => { + if (e.key === 'Enter' || e.key === ' ') { + const l = lineFromEvent(e.target); + if (l !== null) { + e.preventDefault(); + onAddComment(l); // intent only (D6) + } + } + }, + dangerouslySetInnerHTML: { __html: html }, + }), + activeLine !== null + ? React.createElement( + 'div', + { className: 'codev-canvas-overlay' }, + React.createElement(CommentAffordance, { line: activeLine, onActivate: onAddComment }), + activeMarkers.length > 0 + ? React.createElement( + 'ul', + { + className: 'codev-canvas-marker-list', + 'aria-label': `Comments on line ${activeLine + 1}`, + }, + activeMarkers.map((m, i) => + React.createElement( + 'li', + { key: String(i), className: 'codev-canvas-marker' }, + React.createElement('span', { className: 'codev-canvas-marker-author' }, m.author), + `: ${m.text}`, + ), + ), + ) + : null, + ) + : null, + ); +} diff --git a/packages/artifact-canvas/src/components/__tests__/artifact-canvas.test.tsx b/packages/artifact-canvas/src/components/__tests__/artifact-canvas.test.tsx new file mode 100644 index 000000000..2bcf82a9f --- /dev/null +++ b/packages/artifact-canvas/src/components/__tests__/artifact-canvas.test.tsx @@ -0,0 +1,304 @@ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, fireEvent, waitFor, cleanup, act } from '@testing-library/react'; +import * as React from 'react'; +import { ArtifactCanvas } from '../ArtifactCanvas.js'; +import type { ReviewMarker } from '../../types.js'; + +afterEach(cleanup); + +/** + * Stub host (spec D3/D6 + text-as-source-of-truth): file + marker adapters share one text store. + * `markerAdapter.add` serializes a positional `` INTO the text and + * notifies the watcher; `list` derives markers from the current text. This exercises the round-trip + * *through text*, not an in-memory side store. + */ +function makeHost(initial: string) { + let text = initial; + const watchers: Array<(c: string) => void> = []; + const parse = (t: string): ReviewMarker[] => { + const out: ReviewMarker[] = []; + t.split('\n').forEach((ln, i) => { + const m = ln.match(//); + // #857 convention: a REVIEW comment annotates the line ABOVE it (it's written on the next + // line). So the marker's logical line is i-1 — the content block the reviewer commented on. + if (m && i > 0) out.push({ author: m[1], line: i - 1, text: m[2], raw: ln.trim() }); + }); + return out; + }; + const fileAdapter = { + read: vi.fn(async () => text), + watch: vi.fn((_uri: string, cb: (c: string) => void) => { + watchers.push(cb); + return { dispose: vi.fn(() => { const i = watchers.indexOf(cb); if (i >= 0) watchers.splice(i, 1); }) }; + }), + }; + const markerAdapter = { + list: vi.fn(async () => parse(text)), + add: vi.fn(async (_uri: string, line: number, body: string, author: string) => { + const lines = text.split('\n'); + lines.splice(line + 1, 0, ``); + text = lines.join('\n'); + watchers.forEach((cb) => cb(text)); + }), + }; + const themeAdapter = { + resolve: vi.fn((tok: string) => (tok === '--codev-canvas-foreground' ? '#111111' : '')), + onChange: vi.fn(() => ({ dispose: vi.fn() })), + }; + return { text: () => text, watchers, fileAdapter, markerAdapter, themeAdapter }; +} + +describe('ArtifactCanvas (Phase 3)', () => { + it('renders content from FileAdapter and lists markers', async () => { + const host = makeHost('# Title\n\nA paragraph.'); + render(); + await waitFor(() => expect(document.querySelector('h1')).not.toBeNull()); + expect(host.fileAdapter.read).toHaveBeenCalledWith('x'); + expect(host.markerAdapter.list).toHaveBeenCalled(); + }); + + it('hover shows the "+" affordance; clicking emits onAddComment with the 0-based line (scenario 2)', async () => { + const host = makeHost('# Title\n\nA paragraph.'); + const onAddComment = vi.fn(); + render(); + const p = await waitFor(() => { + const el = document.querySelector('p[data-line]'); + if (!el) throw new Error('no paragraph yet'); + return el as HTMLElement; + }); + fireEvent.mouseOver(p); + const plus = await screen.findByRole('button', { name: /add comment on line/i }); + fireEvent.click(plus); + expect(onAddComment).toHaveBeenCalledWith(Number(p.getAttribute('data-line'))); + // The package must NOT write the marker itself (D6 intent-only / invariant scenario 6). + expect(host.markerAdapter.add).not.toHaveBeenCalled(); + }); + + it('is keyboard-activatable: Enter on a focused block emits onAddComment (accessibility AC)', async () => { + const host = makeHost('A paragraph.'); + const onAddComment = vi.fn(); + render(); + const p = await waitFor(() => { + const el = document.querySelector('p[data-line]'); + if (!el) throw new Error('no paragraph yet'); + return el as HTMLElement; + }); + expect(p.tabIndex).toBe(0); // focusable + fireEvent.keyDown(p, { key: 'Enter' }); + expect(onAddComment).toHaveBeenCalledWith(0); + }); + + it('round-trips a marker through text: host add → watch → re-list → marker renders (scenario 3)', async () => { + const host = makeHost('A paragraph.'); // line 0, no markers + const onAddComment = vi.fn((line: number) => { + // Host glue: collect text + write back via MarkerAdapter.add (which serializes into the file text). + void host.markerAdapter.add('x', line, 'please clarify', 'reviewer'); + }); + render(); + const p = await waitFor(() => { + const el = document.querySelector('p[data-line]'); + if (!el) throw new Error('no paragraph yet'); + return el as HTMLElement; + }); + fireEvent.mouseOver(p); + fireEvent.click(await screen.findByRole('button', { name: /add comment on line/i })); + expect(host.markerAdapter.add).toHaveBeenCalled(); + // After the host writes + the watcher fires, the component re-lists and renders the marker. + await waitFor(() => expect(document.querySelector('.codev-canvas-has-marker')).not.toBeNull()); + expect(host.text()).toContain(''); // proves text round-trip + }); + + it('drops an out-of-range marker and warns ONCE even across reloads (deferred #4)', async () => { + const host = makeHost('one line only'); + host.markerAdapter.list = vi.fn(async () => [ + { author: 'a', line: 99, text: 'stale', raw: 'r' } as ReviewMarker, + ]); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + render(); + await waitFor(() => expect(document.querySelector('[data-line]')).not.toBeNull()); + expect(document.querySelector('.codev-canvas-has-marker')).toBeNull(); // not rendered + // A watch refresh re-lists the SAME stale marker — it must not re-warn (warn-once). + await act(async () => { host.watchers.forEach((cb) => cb('one line only')); }); + expect(warn).toHaveBeenCalledTimes(1); + warn.mockRestore(); + }); + + it('surfaces adapter errors via onError and does not throw; failed list keeps prior markers (D2)', async () => { + const host = makeHost('A paragraph.'); + host.markerAdapter.list = vi.fn(async () => { throw new Error('list boom'); }); + const onError = vi.fn(); + const err = vi.spyOn(console, 'error').mockImplementation(() => {}); + render(); + await waitFor(() => expect(onError).toHaveBeenCalled()); + expect(document.querySelector('p[data-line]')).not.toBeNull(); // still rendered, no throw + err.mockRestore(); + }); + + it('disposes the watch subscription on unmount; later emits do not throw (scenario 9)', async () => { + const host = makeHost('A paragraph.'); + const { unmount } = render(); + await waitFor(() => expect(document.querySelector('p[data-line]')).not.toBeNull()); + const disposable = host.fileAdapter.watch.mock.results[0].value as { dispose: ReturnType }; + unmount(); + expect(disposable.dispose).toHaveBeenCalled(); + expect(() => host.watchers.forEach((cb) => cb('changed'))).not.toThrow(); + }); + + it('surfaces a FileAdapter.read rejection via onError without throwing (D2)', async () => { + const host = makeHost('A paragraph.'); + host.fileAdapter.read = vi.fn(async () => { throw new Error('read boom'); }); + const onError = vi.fn(); + const err = vi.spyOn(console, 'error').mockImplementation(() => {}); + expect(() => render()).not.toThrow(); + await waitFor(() => expect(onError).toHaveBeenCalled()); + err.mockRestore(); + }); + + it('surfaces a synchronous FileAdapter.watch() failure via onError without throwing (D2)', async () => { + const host = makeHost('A paragraph.'); + host.fileAdapter.watch = vi.fn(() => { throw new Error('watch boom'); }); + const onError = vi.fn(); + const err = vi.spyOn(console, 'error').mockImplementation(() => {}); + expect(() => render()).not.toThrow(); + await waitFor(() => expect(onError).toHaveBeenCalled()); + expect(document.querySelector('p[data-line]')).not.toBeNull(); // read succeeded → content still renders + err.mockRestore(); + }); + + it('Disposable.dispose is safe to call more than once (idempotent contract, D2)', async () => { + const host = makeHost('A paragraph.'); + const { unmount } = render(); + await waitFor(() => expect(document.querySelector('p[data-line]')).not.toBeNull()); + const disposable = host.fileAdapter.watch.mock.results[0].value as { dispose: () => void }; + unmount(); // dispose() #1 + expect(() => disposable.dispose()).not.toThrow(); // dispose() #2 — safe no-op + }); + + it('surfaces an existing marker author + text via the overlay when its line is active (deferred #4)', async () => { + const host = makeHost('A paragraph.\n'); // marker annotates line 0 + render(); + const p = await waitFor(() => { + const el = document.querySelector('p[data-line]'); + if (!el) throw new Error('no paragraph yet'); + return el as HTMLElement; + }); + await waitFor(() => expect(document.querySelector('.codev-canvas-has-marker')).not.toBeNull()); + fireEvent.mouseOver(p); + const list = await screen.findByLabelText(/comments on line/i); + expect(list.textContent).toContain('bob'); + expect(list.textContent).toContain('please fix'); + }); + + it('activates on Space (not just Enter) on a focused block', async () => { + const host = makeHost('A paragraph.'); + const onAddComment = vi.fn(); + render(); + const p = await waitFor(() => { + const el = document.querySelector('p[data-line]'); + if (!el) throw new Error('no paragraph yet'); + return el as HTMLElement; + }); + fireEvent.keyDown(p, { key: ' ' }); + expect(onAddComment).toHaveBeenCalledWith(0); + }); + + it('keeps previously-rendered markers when a LATER list() rejects (D2 — prior markers preserved)', async () => { + const host = makeHost('A paragraph.\n'); // marker annotates line 0 + const err = vi.spyOn(console, 'error').mockImplementation(() => {}); + render(); + await waitFor(() => expect(document.querySelector('.codev-canvas-has-marker')).not.toBeNull()); + // Now a later list() fails; trigger a watch update (same content). + host.markerAdapter.list = vi.fn(async () => { throw new Error('list boom'); }); + await act(async () => { host.watchers.forEach((cb) => cb('A paragraph.\n')); }); + expect(document.querySelector('.codev-canvas-has-marker')).not.toBeNull(); // prior marker remains + err.mockRestore(); + }); + + it('a slow initial read() does not overwrite newer watch content (request-versioning, iter-2 Codex)', async () => { + const host = makeHost('OLD content.'); + let resolveRead!: (v: string) => void; + host.fileAdapter.read = vi.fn(() => new Promise((res) => { resolveRead = res; })); + render(); + // Newer watch content arrives before the slow read resolves. + await act(async () => { host.watchers.forEach((cb) => cb('NEW content.')); }); + await waitFor(() => expect(document.body.textContent).toContain('NEW content')); + // The stale initial read resolves last — it must NOT overwrite the newer content. + await act(async () => { resolveRead('OLD content.'); }); + expect(document.body.textContent).toContain('NEW content'); + expect(document.body.textContent).not.toContain('OLD content'); + }); + + it('re-fetches when refreshKey changes — the no-watcher refresh contract (D6)', async () => { + const host = makeHost('first.'); + host.fileAdapter.read = vi + .fn() + .mockResolvedValueOnce('first content.') + .mockResolvedValueOnce('second content.'); + const { rerender } = render( + , + ); + await waitFor(() => expect(document.body.textContent).toContain('first content')); + expect(host.fileAdapter.read).toHaveBeenCalledTimes(1); + // Host data changed (no watcher) → it bumps refreshKey to force a refresh. + rerender(); + await waitFor(() => expect(document.body.textContent).toContain('second content')); + expect(host.fileAdapter.read).toHaveBeenCalledTimes(2); + }); + + it('clears a stale activeLine after a reload removes the hovered block (no "+" / no onAddComment on an invalid line, iter-5 Codex)', async () => { + const host = makeHost('# Title\n\nFirst paragraph.\n\nSecond paragraph.'); + const onAddComment = vi.fn(); + render(); + // Hover the LAST block (highest data-line) so the overlay anchors to a line the reload removes. + const blocks = await waitFor(() => { + const els = document.querySelectorAll('p[data-line]'); + if (els.length < 2) throw new Error('paragraphs not rendered yet'); + return els; + }); + const last = blocks[blocks.length - 1]; + fireEvent.mouseOver(last); + expect(await screen.findByRole('button', { name: /add comment on line/i })).not.toBeNull(); + // A watch reload shrinks the document so the previously-hovered block no longer exists. + await act(async () => { host.watchers.forEach((cb) => cb('# Title')); }); + await waitFor(() => expect(document.querySelectorAll('p[data-line]').length).toBe(0)); + // The stale overlay is gone — no "+" on the now-invalid line, and clicking is impossible. + expect(screen.queryByRole('button', { name: /add comment on line/i })).toBeNull(); + expect(onAddComment).not.toHaveBeenCalled(); + }); + + it('KEEPS the active overlay across a reload that still contains the hovered block (no over-clear; guards the CI clobber)', async () => { + // Root cause of the CI-only e2e failure: the iter-5 reset was UNCONDITIONAL on content change, + // so it could wipe a valid `activeLine` (e.g. one set by a hover that raced the initial async + // load). The fix VALIDATES instead: a still-present line survives a reload. This test fails + // against the blind-reset implementation and passes against the validating one. + const host = makeHost('# Title\n\nA paragraph.'); // paragraph at data-line 2 + render(); + const p = await waitFor(() => { + const el = document.querySelector('p[data-line]'); + if (!el) throw new Error('no paragraph yet'); + return el as HTMLElement; + }); + fireEvent.mouseOver(p); + expect(await screen.findByRole('button', { name: /add comment on line/i })).not.toBeNull(); + // A reload to DIFFERENT content where the hovered block (line 2) still exists. + await act(async () => { host.watchers.forEach((cb) => cb('# Title\n\nA paragraph.\n\nMore text.')); }); + await waitFor(() => expect(document.querySelectorAll('p[data-line]').length).toBe(2)); + // The overlay must NOT have been clobbered — the hovered line is still present. + expect(screen.queryByRole('button', { name: /add comment on line/i })).not.toBeNull(); + }); +}); + +describe('ThemeAdapter contract (D4 Model A, scenario 4 — not on the v1 render path)', () => { + it('resolve() returns the host value and onChange returns a disposable that fires', () => { + let fired = false; + const theme = { + resolve: (tok: string) => (tok === '--codev-canvas-accent' ? '#0969da' : ''), + onChange: (h: () => void) => { h(); return { dispose: () => {} }; }, // simulate a theme switch + }; + expect(theme.resolve('--codev-canvas-accent')).toBe('#0969da'); + const sub = theme.onChange(() => { fired = true; }); + expect(fired).toBe(true); + expect(typeof sub.dispose).toBe('function'); + sub.dispose(); // idempotent no-op + }); +}); diff --git a/packages/artifact-canvas/src/index.ts b/packages/artifact-canvas/src/index.ts new file mode 100644 index 000000000..556ea5f43 --- /dev/null +++ b/packages/artifact-canvas/src/index.ts @@ -0,0 +1,20 @@ +/** + * Public API for @cluesmith/codev-artifact-canvas. + * + * Phase 1: locked adapter interfaces + data types + component props, and a placeholder + * `ArtifactCanvas` component. + * Phase 2: the markdown renderer (`renderMarkdown`) + standalone `MarkdownView`. + * + * The default theme stylesheet is a separate export: + * import '@cluesmith/codev-artifact-canvas/default-theme.css'; + */ + +export type { FileAdapter } from './adapters/FileAdapter.js'; +export type { MarkerAdapter } from './adapters/MarkerAdapter.js'; +export type { ThemeAdapter } from './adapters/ThemeAdapter.js'; +export type { Disposable, ReviewMarker, ArtifactCanvasProps } from './types.js'; + +export { ArtifactCanvas } from './components/ArtifactCanvas.js'; + +export { renderMarkdown } from './renderer/renderer.js'; +export { MarkdownView, type MarkdownViewProps } from './renderer/MarkdownView.js'; diff --git a/packages/artifact-canvas/src/overlays/CommentAffordance.tsx b/packages/artifact-canvas/src/overlays/CommentAffordance.tsx new file mode 100644 index 000000000..4334b0c5f --- /dev/null +++ b/packages/artifact-canvas/src/overlays/CommentAffordance.tsx @@ -0,0 +1,28 @@ +import * as React from 'react'; + +export interface CommentAffordanceProps { + /** 0-based source line this affordance targets. */ + line: number; + /** Invoked on click / keyboard activation with the 0-based line (the D6 intent seam). */ + onActivate: (line: number) => void; +} + +/** + * The hover/focus "+" comment affordance (spec D6). It is a real `