[Spec 945] Foundational reusable package @cluesmith/codev-artifact-canvas#1027
Open
amrmelsayed wants to merge 66 commits into
Open
[Spec 945] Foundational reusable package @cluesmith/codev-artifact-canvas#1027amrmelsayed wants to merge 66 commits into
amrmelsayed wants to merge 66 commits into
Conversation
…AC contradiction - Add D7 + Security Considerations: markdown-it html:false + DOMPurify sanitize before render; dompurify dep; new AC + Test Scenario 8; cites #48 precedent. - Resolve D6-vs-AC/Scenario-3 contradiction: package is intent-only (emits onAddComment(line)); host owns input + calls MarkerAdapter.add. Fixed D3, AC item, Test Scenario 3, and interface annotation to match. - Fold cheap Claude items: 0-based ReviewMarker.line; blockquotes/tables in AC; Disposable-teardown Test Scenario 9. - Correct Consultation Log + thread to reflect actual verdicts (Gemini+Codex REQUEST_CHANGES, Claude APPROVE) instead of the earlier inaccurate summary.
…esolve ThemeAdapter/CSS - D2: separate async (read + MarkerAdapter) from sync (watch/onChange return Disposable immediately) — fixes the iter-2 Codex async/sync contradiction. - Add ArtifactCanvasProps to the locked interface block: canonical onAddComment(line: number) intent seam (+ adapters, uri, onError). D6/AC/Test reference it. Exported from public API. - D4 Model A: CSS variables are the sole v1 theming mechanism + enumerated token vocabulary; ThemeAdapter.resolve()/onChange are JS-side (#863 canvas), off the v1 render path. Test 4 reframed. - D6: component auto-re-calls MarkerAdapter.list on watch (host doesn't retrigger). - New AC: hover-+ is keyboard-accessible (Enter/Space + SR label). - Consultation Log: honest iter-2 verdicts + iter-3 changes.
…d e2e test - Phase 1: add repo build/test/release wiring deliverables (root package.json build+test, scripts/bump-all.sh) + acceptance criterion — fixes Codex #1 (package would otherwise be an orphan, not in the build/version flow). - Phase 4: lock an automated end-to-end round-trip test at src/__tests__/end-to-end.test.tsx (Vitest+TL+stub fixtures) as the primary contract proof; examples/ Vite page is now a dev aid — fixes Codex #2. - Fold Claude notes: jsdom test env, tsup-rationale README note, optional React-18 smoke, P3-density escape hatch. Add plan Consultation Log.
…nder path + e2e round-trips through text - P3: themeAdapter accepted as a prop but NOT subscribed/used for v1 render (CSS-var theming only, D4 Model A); only FileAdapter.watch subscribed; resolve/onChange exercised solely by the scenario-4 contract test. Fixes Codex iter-2 #1. - P4: e2e test now requires stub MarkerAdapter.add to serialize a positional REVIEW marker into the markdown string; read/watch/list derive from that text (not in-memory). Fixes Codex iter-2 #2 (text round-trip). - Fold Claude nits: accurate root build-script form + insertion point; root test convention (per-package + CI); @types/markdown-it/@types/dompurify + vite devDeps; tsconfig base; scenario-6 invariant echo.
… contradiction + name CI/release wiring - Resolve P1 test-wiring self-contradiction: root build includes the package; root test is NOT extended; package's own test runs in CI. AC reworded to match. - Name .github/workflows/test.yml explicitly (dedicated artifact-canvas step) — the repo's actual CI mechanism, not the root test script. - Add release-protocol update deliverable + explicit publish decision (not independently npm-published in v1; bundled by hosts via workspace:*). - Fold Claude tsconfig note (override module/moduleResolution to ESNext/bundler).
…tics + out-of-range-marker policy - P3: add explicit adapter error-semantics deliverable + AC + tests (spec D2 locked: guarded read/watch/list/resolve/onChange → onError + console; failed list() preserves prior markers). Fixes Codex iter-4 #1. - P3: resolve the spec-deferred out-of-range-marker policy — ignore + warn once (over clamp/hard-error) + test. Fixes Codex iter-4 #2. - P4: name examples/ entrypoint (index.html + main.tsx + vite.config.ts, dev:example script). Fold Claude notes: CI step placement, publish-analogy precision, vite devDep timing.
…release git-add blocks - P3: the v1 component guards only the calls it makes (read/watch/list); ThemeAdapter.resolve/onChange are not called by it (D4 Model A) so they're out of its error scope — error handling belongs to the scenario-4 contract test / #863 consumer. Removes the iter-5 self-contradiction. AC updated. - P1 release deliverable: also update the hardcoded stable/RC git-add command blocks to stage packages/artifact-canvas/package.json (not just the enumeration). Per human decision after iter-5: fix these two and take the plan to the plan-approval gate (no 6th consult).
…canvas package
Phase 1 of the artifact-canvas plan: package skeleton, dual-format build, locked
adapter interfaces, theme tokens, and repo wiring.
Package (packages/artifact-canvas/):
- package.json (@cluesmith/codev-artifact-canvas 3.1.7; peer react/react-dom ^18||^19;
deps markdown-it + dompurify; dual CJS+ESM exports + ./default-theme.css)
- tsup dual-format build (CJS+ESM+dts, react external, copies default-theme.css)
- tsconfig (extends config base; ESNext/bundler/react-jsx; jsdom vitest)
- adapters/{FileAdapter,MarkerAdapter,ThemeAdapter}.ts — interfaces only
- types.ts: ReviewMarker, Disposable, ArtifactCanvasProps (onAddComment intent seam)
- default-theme.css (8 --codev-canvas-* tokens), ArtifactCanvas placeholder, index.ts
- tests: import-boundary (no vscode/node/fs/fetch in shipped src); build-smoke script
Repo wiring:
- root build script (artifact-canvas after core, before codev)
- scripts/bump-all.sh (version-aligned bump loop)
- .github/workflows/test.yml (build+smoke and unit-test steps)
- release protocol (enumeration + stable/RC git-add blocks; publish untouched — not v1-published)
Verified: package build+tests+smoke green; full root build green; codev suite green.
…p injectable-logger; resolve() full property name) Implement consult iter-1 (Codex) caught that the code was correct but the spec prose was stale on two deferred-item decisions the plan mandated syncing: - D2: drop the 'injectable logger' claim → 'logged to the console' + onError? (deferred #1). Also dropped ThemeAdapter.resolve/.onChange from D2's guarded-calls list for consistency with D4 Model A (v1 component doesn't call them). - ThemeAdapter.resolve interface comment: full property name example resolve('--codev-canvas-foreground') instead of ('foreground') (deferred #2).
…e protocol iter-2 Codex (COMMENT): the RC-publishing comment still said only codev/core/types are bumped; artifact-canvas is now in the RC bump + git-add. Sync the comment.
…urce-mapping + DOMPurify sanitization Phase 2: the markdown rendering layer. - src/renderer/renderer.ts: markdown-it (html:false, linkify) + a codev_data_line core rule stamping 0-based token.map[0] onto block tokens (headings, paragraphs, lists, list items, blockquotes, fenced/indented code, tables); output sanitized via DOMPurify before return. data-* attrs (incl. data-line) preserved. - src/renderer/MarkdownView.tsx: React component rendering the sanitized HTML. - Export renderMarkdown + MarkdownView from index.ts. - Tests (13/13): data-line attribution across block types; sanitization. Plan deviation (documented): deferred-#3's 'javascript: link proves DOMPurify' premise is wrong (markdown-it validateLink neutralizes javascript: before DOMPurify), so the sanitize step is proven via a DOMPurify.sanitize spy instead; all 3 defense layers kept.
porch's tests check runs the whole codev suite; it failed on pre-existing flaky tests (non-deterministic: 7 failures full-suite → 1 on isolated re-run; 0 in Phase 1), all in codev files unrelated to the artifact-canvas work. Quarantined per the builder protocol's Handling Flaky Tests rule (architect-authorized) with describe.skip + // FLAKY annotations: - tunnel-integration.test.ts (file-watcher timing) - default-branch.test.ts (git-fixture isolation) - non-main-default-branch.test.ts (git-fixture isolation; all 3 describes) - team-cli.test.ts (afx team deprecation only; spy ordering) Documented in codev/reviews/945-*.md (Flaky Tests). Architect filing a tracker for the underlying fix so the skips are not permanent.
…for out-of-range markers Codex iter-3 (REQUEST_CHANGES, MEDIUM): out-of-range markers were warned on every list() reload, but the deferred-#4 AC says 'dropped and warned once'. Added a warnedRef Set to dedup the console.warn per unique stale marker; tightened the test to assert warn fires exactly once across a watch reload. 28/28 green.
…r no-watcher refresh (D6) Codex iter-4 (HIGH): spec D6 promised a no-watcher host could force refresh by re-rendering, but the effect's stable deps mean a same-props re-render does not re-fetch (correct React). Architect-authorized additive fix: - ArtifactCanvasProps gains an OPTIONAL refreshKey?: number | string (no-op default; hosts with a watcher omit it → behavior unchanged). Added to the effect deps so bumping it forces a fresh read + marker-list. - Corrected spec D6 wording to name refreshKey as the contract (host bumps it on data change; plain re-render does not re-fetch). - Test: refreshKey 1->2 triggers a second read + new content (no watcher involved). 29/29.
…eload; pin out-of-range marker channel to console.warn iter-5 Codex (REQUEST_CHANGES): - Stale activeLine: activeLine persisted across watch/refreshKey reloads without validation, so the overlay could render + on — and emit onAddComment for — a line the reloaded document no longer contained. Fixed at root: reset activeLine on every content change. Regression test: after a reload that removes the hovered block, no + shows and onAddComment never fires for the removed line. - Out-of-range marker channel: plan wording was ambiguous (onError?/console.warn). Tightened to console.warn-only; onError? is reserved for genuine adapter failures (an out-of-range marker is data-hygiene, not a failure). Architect-authorized plan-wording tightening (same basis as the iter-4 D6 spec tightening).
…tiveLine fix; out-of-range channel tightening)
…ples dev page, README - src/__tests__/fixtures/: stub adapters (createStubHost + named stub*Adapter factories over a shared text store; text-as-source-of-truth, #857) + a realistic sample artifact with a seeded REVIEW marker. - src/__tests__/end-to-end.test.tsx: the primary contract proof — render → existing marker → hover/click + AND focus/Enter → onAddComment (0-based) → host add serializes INTO text → watch replays → list re-derives → new marker renders. Asserts the marker lands in the text store (round-trip THROUGH text, not a UI-only refresh). - examples/: Vite dev page reusing the same stubs (pnpm dev:example); excluded from the package. - README.md: 3 adapter contracts, ArtifactCanvasProps, --codev-canvas-* tokens + override, host walkthrough, why-tsup rationale, scope. build 0, 33/33 tests, check-types 0, examples bundles headlessly, dist clean (no test/example leak).
…e:* (not npm-published in v1) iter-1 Codex (HIGH): README told consumers to 'pnpm add @cluesmith/codev-artifact-canvas', but v1 is not independently npm-published (per spec-945 / release/protocol.md) — it's consumed via workspace:* and bundled by hosts. Rewrote the Install section to add it as a host workspace dependency; removed all npm-install guidance. Docs-only.
…summary, lessons, arch/lessons updates)
… the review record
…stead of blind reset (fixes CI-only e2e race) The iter-5 stale-activeLine reset (useEffect(() => setActiveLine(null), [content])) was unconditional. The initial async load sets content outside the test's act(), so that effect could fire AFTER a hover set activeLine, clobbering it — the + overlay never mounted and the Phase 4 MOUSE e2e test's findByRole timed out. Reproduced only under CI's slower scheduling (local passes 10x full-suite). Fix: validate the overlay anchor against the reloaded DOM (clear activeLine only when its [data-line] element is genuinely absent), folded into the decoration effect keyed on [html]. A valid hovered line survives (no clobber); a removed/shortened block still clears (preserves the iter-5 Codex intent). Added a deterministic regression test proven to fail against the old blind-reset and pass against the validation fix. 34/34 green.
…plan approved (PR consult items) PR consult (re-run on final code) Codex REQUEST_CHANGES, both fixed: - Version lockstep: artifact-canvas was 3.1.7 while the workspace is 3.1.9 (rebase side effect); bumped to 3.1.9 to restore the invariant the release/bump wiring relies on. Lockfile unchanged. - Approved plan artifact still said 'Status: draft' with no approval frontmatter though status.yaml shows plan-approval approved; added frontmatter + flipped Status to approved. No code-behavior change; build + 34 tests green.
… consultation tally (PR iter-2 COMMENTs)
…I-only keyboard test race) 3rd CI-only failure (Phase 3 keyboard test, 'expected -1 to be +0'). Evidence shows the failing assertion is expect(p.tabIndex).toBe(0), NOT onAddComment: markdown-it deterministically yields data-line='0' (proven; data-line.test passes in the same CI run), so onAddComment(-1) is impossible; -1 is a <p>'s default tabIndex. The decoration effect set tabIndex=0 AFTER render and the test read it synchronously when the element appeared, racing the effect under CI's slower scheduling (same race family as the e2e activeLine fix). Fix: stamp tabindex='0' at render time in the renderer core rule (alongside data-line), so focusability is in the HTML the instant a block mounts. Removed the redundant el.tabIndex=0 from the decoration effect. Added a deterministic renderer test for render-time tabindex. Preempts the same latent race in the e2e keyboard test. types 0, build 0, 35/35 ×12 runs green.
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
Builds
@cluesmith/codev-artifact-canvas(packages/artifact-canvas/) — a host-agnostic React library for rendering and reviewing Codev markdown artifacts (specs/plans/reviews) across surfaces (VSCode, dashboard, future mobile). Delivered over 4 SPIR implement phases.This package ships standalone — no host integration (that is #859 and follow-ups). Hosts wire it up by implementing three small adapter interfaces.
Closes #945.
What's included
.d.ts) viatsup; React externalized as a peer (^18 || ^19).FileAdapter(read + watch),MarkerAdapter(list/add),ThemeAdapter(JS-side, off the v1 render path).markdown-itwithhtml:false+ DOMPurify sanitization, 0-baseddata-lineattribution on block tokens.ArtifactCanvascomponent: intent-only comment overlay (emitsonAddComment(line), never writes markers itself), minimal v1 marker rendering, adapter-driven data flow with request-versioning, warn-once out-of-range handling, a no-watcherrefreshKeyrefresh path, and overlay-anchor reconciliation against the reloaded DOM. Keyboard-accessible (focusable blocks, Enter/Space, ARIA).--codev-canvas-*tokens + a./default-theme.cssexport.examples/Vite dev page, and a comprehensiveREADME.md..github/workflows/test.yml), lockstep bump (scripts/bump-all.sh), and release-protocol enumeration. Not independently npm-published in v1 — consumed viaworkspace:*and bundled by hosts.Tests
34 passing in the package (5 files: data-line, sanitization, artifact-canvas, import-boundary, end-to-end). Build, type-check, and a headless
examples/bundle all verified. All 6 CI jobs green.Notes for the reviewer
@cluesmith/codevtests (unrelated to this package) are.skip-quarantined on this branch per the builder protocol's flaky-test rule (architect-authorized). See the review's Flaky Tests section — un-skip tracked as a follow-up.main(then at vscode: Forward file/symbol/hunk/selection references into the builder PTY (#789) #1023). A CI-only e2e race (an unconditionalactiveLinereset that could clobber a hover when the initial async load's effect landed late under CI's slower scheduling) was fixed by validating the overlay anchor against the reloaded DOM instead; covered by a deterministic regression test.Artifacts
codev/specs/945-build-foundational-reusable-pa.mdcodev/plans/945-build-foundational-reusable-pa.mdcodev/reviews/945-build-foundational-reusable-pa.md