Skip to content

canvas migration PR1+PR2+PR3 — facade, ProcessMapBase absorption, FrameView shell#126

Merged
jukka-matti merged 4 commits into
mainfrom
codex/canvas-migration-pr1-pr3
May 5, 2026
Merged

canvas migration PR1+PR2+PR3 — facade, ProcessMapBase absorption, FrameView shell#126
jukka-matti merged 4 commits into
mainfrom
codex/canvas-migration-pr1-pr3

Conversation

@jukka-matti
Copy link
Copy Markdown
Owner

Summary

Canvas migration phases 1–3 of the eight-phase strangler-pattern delivery from docs/superpowers/specs/2026-05-04-canvas-migration-design.md. Three sequential commits, single PR per feedback_slice_size_cap exception (each phase is small, self-contained, and forms one coherent surface unification).

Phase Commit What
PR1 2df0362e <Canvas> component as a thin facade delegating to LayeredProcessViewWithCapability. Both apps' FrameView swaps mount to <Canvas>. No behavior change — users see identical UI.
PR2 25769d93 ProcessMapBase rendering moves inside Canvas as Canvas/internal/ProcessMapBase.tsx. Public ProcessMap/ProcessMapBase.tsx becomes a deprecated re-export. ADR-076's b0 branching collapses into Canvas's "is map empty?" check.
PR3 b987dbca CanvasWorkspace shared component absorbs the canvas-mount + filter-state plumbing. Both apps' FrameView reduces to thin route shells. Synthetic-investigation useState workaround replaced by session-scoped useSessionCanvasFilters hook (per Canvas Migration spec Decision 2 — three-layer state; canvas filters live in View layer).

Architectural decisions honored

  • Strangler-pattern phased delivery (Canvas Migration spec Decision 1) — three small commits replace big-bang
  • Three-layer state separation (Decision 2) — useSessionCanvasFilters puts filter state in the View layer (session-scoped, transient) instead of investigation-bound persistence
  • b0 collapse into Canvas (Decision 3) — supersedes ADR-076; b0 ceases to exist as separate render path after PR2
  • feedback_slice_size_cap — each phase is independently revertable; bundled here because all three are mechanical no-behavior-change refactors

Pre-merge gate

bash scripts/pr-ready-check.sh ✅ green per Codex's verification:

  • Tests: all packages green
  • Lint: clean (warnings only — pre-existing baseline)
  • docs:check: passed
  • PWA build: clean
  • dist-integrity: OK

What's NOT in this PR

  • Spec 2 design doc (Manual Canvas Authoring) — landed separately on main as 01de9177 ahead of this PR
  • PR4 (Spec 2 implementation) — separate brainstorm + plan + PR per the migration sequence
  • PR8 cleanup — final removal of legacy LayeredProcessView / ProcessMapBase / FrameView files happens after Specs 2–5 merge

Test plan

  • CI green
  • Manual claude --chrome walk PWA: paste sample → land on FrameView (now <Canvas> facade) → all existing FRAME interactions work identically
  • Manual claude --chrome walk Azure: same end-to-end check
  • Spot-check: existing Pareto / capability / outcome-pin behaviors unchanged
  • Verify useSessionCanvasFilters clears filter state on reload (View-layer transience)
  • Code-review pass (Opus) — focus on facade prop forwarding correctness + PR3 session-state migration

@vercel
Copy link
Copy Markdown

vercel Bot commented May 5, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
mean-beoynd-lite-pwa Ready Ready Preview, Comment May 5, 2026 7:59am
variscout_website Ready Ready Preview, Comment May 5, 2026 7:59am

@jukka-matti
Copy link
Copy Markdown
Owner Author

Code + Spec Review — fixes requested before merge

Two parallel reviewers (spec-compliance + code-quality) ran against codex/canvas-migration-pr1-pr3. Pre-merge gate green, tests green, three sub-PRs functionally correct. Two merge blockers + four recommend-fixes:

Blockers (must fix before merge)

  1. packages/ui/src/components/Canvas/internal/ProcessMapBase.tsx:79Math.random() violates project hard rule.
    const uid = (prefix) => \${prefix}-${Math.random().toString(36).slice(2, 8)}-${Date.now().toString(36)}`Replace withcrypto.randomUUID()(already established convention for client-side IDs per MEMORY.md). The absorption is the right moment to clean this up — it was inherited from the originalProcessMap/ProcessMapBase.tsx` but breaks test determinism and contradicts the deterministic-engine principle.

  2. PWA apps/pwa/src/components/views/FrameView.tsx:9 — imports useProjectStore from @variscout/stores.
    apps/pwa/CLAUDE.md hard rule states: "State via React Context (DataContext). No Zustand stores in PWA."
    The PWA may already have invariant drift elsewhere (App.tsx uses useProjectStore), but this is the first FrameView wiring full Zustand-store access. Decision needed:

    • (a) The DataContext invariant is superseded — confirm and update apps/pwa/CLAUDE.md to reflect the new pattern, OR
    • (b) Revert this FrameView to a DataContext-based pattern.
      Should NOT be left as an undocumented invariant violation.

Recommend-fix (strongly suggested before merge)

  1. packages/ui/src/components/Canvas/CanvasWorkspace.tsx:173–185as unknown as ProcessHub cast.
    Cleanest fix: narrow the useProductionLineGlanceData prop type to only the fields it actually reads, instead of casting a stub to the full ProcessHub type.

  2. packages/ui/src/components/Canvas/internal/ProcessMapBase.tsx:410+ — hardcoded palette colors.
    Uses bg-blue-50, bg-amber-50, text-amber-900, text-red-700 directly. Violates @variscout/ui CLAUDE.md "Use semantic Tailwind classes (bg-surface-secondary, text-content, border-edge) that adapt to data-theme." Same "absorbed pattern, fix at the seam" reasoning as chore(deps): bump pnpm/action-setup from 4.1.0 to 4.4.0 #1.

  3. packages/ui/src/components/Canvas/index.tsx — no JSDoc on Canvas / CanvasProps.
    Spec 2 Decision G calls for CRDT-readiness constraints to be documented from day 1. Add a file-level + props JSDoc block (purpose, b0/b1 routing contract, CRDT-ready state expectations).

  4. packages/ui/CLAUDE.md stale note — currently says "In PR1 it is a thin facade over LayeredProcessViewWithCapability". By PR3 the relationship is inverted (Canvas is the implementation; LayeredProcessViewWithCapability is the deprecated re-export stub). Update to: "Canvas is the canonical implementation; LayeredProcessViewWithCapability and ProcessMapBase are deprecated re-export stubs."

Strengths (worth recording)

  • ✅ ADR-076 supersession correctly implemented (frontmatter + body note + superseded-on: 2026-05-04)
  • useSessionCanvasFilters is clean, Decision-2-correct (session-scoped, plain useState, no persistence leak)
  • ✅ FrameView slimming dramatic + intentional (363→44 LOC PWA, 365→44 LOC Azure — byte-for-byte identical between apps; correct deduplication via CanvasWorkspace)
  • ✅ b0 routing absorbed into CanvasWorkspace's scope === 'b0' check (Decision 3 honored)
  • ✅ Test discipline (vi.mock ordering, hoist closure compliance, no shared refs)

Side note — PR4 plan grounding (not your fix to make)

The review surfaced two PR4 plan revisions needed (separate from this PR's fixes):

a. Canvas.mode: 'spatial' | 'full' (existing Operations-band toggle) collides with PR4's planned mode: 'author' | 'read' (authoring mode toggle). Resolution: rename Canvas.modeCanvas.opsMode (more specific) so PR4 can use mode for author/read.

b. PR4's pseudocode uses canonicalMap = { steps, arrows, assignments } shape; actual ProcessMap is { version, nodes, tributaries, ... }. Resolution: align PR4 with existing ProcessMap; treat CanonicalMap as a V2 cleanup.

Both PR4 plan revisions land on main separately; not blocking #126.


Once #1 + #2 are resolved (and ideally #3-#6), this is ready to squash-merge. Codex can ship the fixes on the same branch — small focused commits.

jukka-matti added a commit that referenced this pull request May 5, 2026
@jukka-matti
Copy link
Copy Markdown
Owner Author

Update — PWA Zustand blocker dropped (ADR-078 supersedes the rule)

The "PWA useProjectStore import violates apps/pwa/CLAUDE.md" blocker from the prior comment is dropped. Brainstorm at 2026-05-05 surfaced that the rule was demonstrably false (PWA already uses 4+ Zustand stores across 15+ files; the rule was an aspirational invariant that drifted silently as the PWA matured through slices 1–4).

ADR-078 locks the architectural framing as α — same product, gated tiers: PWA + Azure share state architecture (domain Zustand stores); persistence is the tier gate (PWA opt-in IndexedDB Hub-of-one + .vrs vs Azure Blob Storage sync); tier-gated features check isPaidTier() at mount. The "DataContext only, no Zustand" rule is retired. apps/pwa/CLAUDE.md is updated to match (commit 61fd58a3 on main).

Updated fix list for #126

Blocker (must fix)

  1. packages/ui/src/components/Canvas/internal/ProcessMapBase.tsx:79Math.random()crypto.randomUUID(). Project hard rule + test determinism.

Recommend-fix (strongly suggested before merge)

  1. packages/ui/src/components/Canvas/CanvasWorkspace.tsx:173–185as unknown as ProcessHub cast. Narrow useProductionLineGlanceData prop type to only the fields it reads.
  2. packages/ui/src/components/Canvas/internal/ProcessMapBase.tsx:410+ — hardcoded palette colors (bg-blue-50, text-amber-900, etc.) violate @variscout/ui semantic-Tailwind hard rule. Replace with bg-surface-secondary / text-content / border-edge etc.
  3. packages/ui/src/components/Canvas/index.tsx — no JSDoc on Canvas / CanvasProps. Add file-level + props block (purpose, b0/b1 routing contract, CRDT-readiness expectations per Spec 2 Decision G).
  4. packages/ui/CLAUDE.md stale note — currently says "In PR1 it is a thin facade over LayeredProcessViewWithCapability". Update to: "Canvas is the canonical implementation; LayeredProcessViewWithCapability and ProcessMapBase are deprecated re-export stubs."

After these land, this PR is ready to squash-merge.

Side note — for Codex's awareness, not your fix

PR4 plan now includes two grounding revisions (commit e543602b on main):

  • R1: rename Canvas.modeCanvas.opsMode (frees mode for the B4 author/read toggle in PR4)
  • R2: align canvas state with existing ProcessMap shape (extend with assignments, arrows, parentStepId); no new CanonicalMap type

These are PR4's prerequisites; they don't affect this PR. Just flagging so the rename in #126's surface (if you happen to touch it during the recommend-fixes) doesn't surprise you when PR4 starts.

@jukka-matti jukka-matti force-pushed the codex/canvas-migration-pr1-pr3 branch from b987dbc to b0da708 Compare May 5, 2026 07:56
@jukka-matti
Copy link
Copy Markdown
Owner Author

Implemented the #126 review fixes on codex/canvas-migration-pr1-pr3.

Fixed:

  • Replaced Canvas ProcessMapBase ID generation with crypto.randomUUID().
  • Narrowed production-line glance hub typing so CanvasWorkspace no longer casts a preview stub with as unknown as ProcessHub.
  • Replaced hardcoded Canvas process-map palette classes with semantic theme classes.
  • Added Canvas file/API JSDoc covering the canonical surface, controlled-props contract, b0/b1 workspace routing, and CRDT-ready state boundary.
  • Updated packages/ui/CLAUDE.md to reflect Canvas as the canonical implementation and legacy wrappers as deprecated compatibility surfaces.

Verification:

  • pnpm --filter @variscout/core test -- stepErrorAggregation.test.ts
  • pnpm --filter @variscout/hooks test -- useProductionLineGlanceData.test.ts useSessionCanvasFilters.test.ts
  • pnpm --filter @variscout/ui test -- Canvas.test.tsx CanvasProcessMap.test.tsx CanvasWorkspace.test.tsx LayeredProcessViewWithCapability.test.tsx
  • pnpm --filter @variscout/ui build
  • pnpm --filter @variscout/pwa build
  • pnpm --filter @variscout/azure-app build
  • rg "Math\.random" packages/ui/src/components/Canvas — no matches
  • rg "as unknown as ProcessHub" packages/ui/src/components/Canvas packages/hooks/src — no matches
  • bash scripts/pr-ready-check.sh — passed

Ready for re-review.

@jukka-matti jukka-matti merged commit ef8335c into main May 5, 2026
3 checks passed
@jukka-matti jukka-matti deleted the codex/canvas-migration-pr1-pr3 branch May 5, 2026 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant