Skip to content

Comments

feat(edit): rebuild packages/edit on rafters#108

Closed
ssilvius wants to merge 6 commits intomainfrom
feat/edit-rebuild
Closed

feat(edit): rebuild packages/edit on rafters#108
ssilvius wants to merge 6 commits intomainfrom
feat/edit-rebuild

Conversation

@ssilvius
Copy link
Contributor

Summary

  • Rebuild packages/edit as a thin orchestration layer on top of @rafters/ui instead of reimplementing editor primitives
  • 7 block types (heading, paragraph, list, image, embed, divider, form) with Zod schemas and renderers
  • Composed PageEditor wiring BlockCanvas + Sidebar + PropertyEditor + Toolbar from rafters
  • Unified blockConfigs registry (single source of truth for definitions, schemas, defaults)
  • useAutoSave hook with debounced persistence and skip-if-unchanged
  • PageService CRUD with dependency-injected PageStorage backend
  • Block[] ↔ JSON serialization with Zod validation
  • 76 tests across 7 test files, all passing

Test plan

  • pnpm --filter @phantom-zone/edit test — 76 tests pass
  • pnpm --filter @phantom-zone/edit exec tsc --noEmit — typecheck clean
  • pnpm biome check packages/edit/ — lint clean
  • pnpm --filter @phantom-zone/edit build — ESM + DTS output
  • Verify integration in dashboard app

🤖 Generated with Claude Code

Rebuild the edit package from scratch as a thin layer on top of @rafters/ui,
rather than reimplementing editor primitives. All editor components, hooks,
and block model come from rafters; this package provides PZ-specific block
definitions, renderers, persistence, serialization, and page management.

Modules:
- blocks/: 7 block types (heading, paragraph, list, image, embed, divider, form)
  with Zod schemas, BlockDefinitions, and default props
- editor/: PageEditor component wiring BlockCanvas + Sidebar + PropertyEditor +
  Toolbar; usePageEditorState hook with undo/redo; unified blockConfigs registry
- serialization/: Block[] <-> JSON with Zod validation
- persistence/: PagePersistence interface + useAutoSave hook (debounced, skip-if-unchanged)
- pages/: PageService CRUD with PageStorage interface for DI

76 tests across 7 test files. Ambient .d.ts declarations prevent TS from
walking into linked rafters source (avoids OOM, Zod version mismatch).

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings January 27, 2026 09:59
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR rebuilds packages/edit as a thin orchestration layer on top of @rafters/ui, replacing a previous implementation that reimplemented editor primitives. The new implementation provides 7 block types (heading, paragraph, list, image, embed, divider, form) with Zod schemas, a composed PageEditor component, unified block registry, auto-save functionality, and page CRUD operations.

Changes:

  • New packages/edit package with complete rebuild of page editor functionality on rafters foundation
  • Block definitions with Zod schemas for heading, paragraph, list, image, embed, divider, and form types
  • State management, persistence, serialization, and page service infrastructure with 76 passing tests

Reviewed changes

Copilot reviewed 35 out of 38 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
packages/edit/package.json New package configuration with rafters dependency and test setup
packages/edit/tsconfig.json TypeScript configuration with JSX and paths setup
packages/edit/tsup.config.ts Build configuration for ESM output with type definitions
packages/edit/vitest.config.ts Test configuration using happy-dom environment
packages/edit/src/types/rafters.d.ts Ambient type declarations for rafters API surface
packages/edit/src/blocks/*.ts Block type definitions with Zod schemas and defaults for 7 block types
packages/edit/src/editor/PageEditor.tsx Main composed editor component wiring rafters primitives
packages/edit/src/editor/renderers.tsx Block renderer implementations for each block type
packages/edit/src/editor/registry.ts Unified block registry with schemas and defaults
packages/edit/src/editor/state.ts Page editor state management hook with undo/redo
packages/edit/src/persistence/auto-save.ts Auto-save hook with debouncing and change detection
packages/edit/src/pages/page-service.ts Page CRUD service with storage abstraction
packages/edit/src/serialization/json.ts JSON serialization with Zod validation
packages/edit/src/index.ts Main package exports
packages/edit/test/*.test.ts 7 test files with 76 tests covering all major functionality
tsconfig.json Added packages/edit to workspace paths and references
pnpm-lock.yaml Added dependencies for packages/edit including testing libraries
biome.json Updated file includes for new package structure
apps/dashboard/package.json Added @phantom-zone/edit dependency
.serena/memories/architecture.md Updated architecture documentation describing rafters integration
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +153 to +159
children: original.children
? original.children.map((child) => ({
...child,
id: generateId(),
props: structuredClone(child.props),
}))
: undefined,
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The duplicateBlock function only recursively clones children at depth 1 (lines 153-159). When mapping over original.children, it creates new IDs and clones props for each child, but if those children have nested children of their own, those nested grandchildren won't get new IDs.

This could lead to duplicate block IDs in the document tree when duplicating blocks with deeply nested children, which would violate the uniqueness constraint for block IDs.

Consider implementing a recursive clone function that handles arbitrary nesting depth:

function cloneBlockWithNewIds(block: Block): Block {
  return {
    ...block,
    id: generateId(),
    props: structuredClone(block.props),
    children: block.children?.map(cloneBlockWithNewIds),
  };
}

Copilot uses AI. Check for mistakes.
"zod": "^4.0.0"
},
"dependencies": {
"@phantom-zone/core": "workspace:*",
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The package.json declares @phantom-zone/core as a dependency but it's not imported or used anywhere in the package source code. This is unnecessary cruft that adds to the bundle size and dependency tree without providing value.

Consider removing this dependency unless there are plans to use it in the near future.

Suggested change
"@phantom-zone/core": "workspace:*",

Copilot uses AI. Check for mistakes.
├── blocks/ - Block type definitions + renderers (wrapping rafters components)
├── editor/ - Composed PageEditor wiring rafters components together
├── persistence/ - Auto-save + versioning via @phantom-zone/storage
├── serialization/ - MDX import/export
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The architecture documentation states that serialization should support "MDX import/export" (line 46 in the old version), but the actual implementation only provides JSON serialization/deserialization.

There's no MDX support in the codebase - only serializeBlocks and deserializeBlocks which work with plain JSON. If MDX support is planned for the future, this should be noted as "TODO" in the documentation. If it's not planned, the documentation should be updated to reflect the actual implementation (JSON only).

Suggested change
├── serialization/ - MDX import/export
├── serialization/ - JSON import/export (MDX support TODO)

Copilot uses AI. Check for mistakes.
import { describe, expect, it, vi } from "vitest";
import type { PageStorage } from "../src/pages/page-service";
import { PageService } from "../src/pages/page-service";
import { serializeBlocks } from "../src/serialization/json";
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test imports serializeBlocks from "../src/serialization/json" which bypasses the public API exported from "../src/serialization/index.ts".

While this works, it's better practice for tests to import from the same paths that external consumers would use. This ensures the public API is properly tested and makes refactoring easier.

Consider changing the import to:

import { serializeBlocks } from "../src/serialization";
Suggested change
import { serializeBlocks } from "../src/serialization/json";
import { serializeBlocks } from "../src/serialization";

Copilot uses AI. Check for mistakes.
},
"dependencies": {
"@phantom-zone/core": "workspace:*",
"@rafters/ui": "link:../../../rafters/packages/ui"
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The @rafters/ui dependency uses a link: protocol pointing to a sibling repository at ../../../rafters/packages/ui. This creates a brittle coupling where:

  1. The relative path assumes a specific directory structure outside this repository
  2. The dependency won't resolve for other developers or CI unless they have the rafters repository checked out in the exact expected location
  3. This breaks the package's portability and makes it difficult to publish or consume

Consider either:

  • Publishing @rafters/ui to a registry (npm, private registry, or GitHub packages) and using a normal version specifier
  • Using pnpm workspace protocol if rafters is intended to be a monorepo workspace
  • Adding clear documentation about the expected repository layout and setup instructions
Suggested change
"@rafters/ui": "link:../../../rafters/packages/ui"
"@rafters/ui": "workspace:*"

Copilot uses AI. Check for mistakes.
Comment on lines +8 to +21
export const SerializedBlockSchema = z.object({
id: z.string().min(1),
type: z.string().min(1),
props: z.record(z.string(), z.unknown()),
children: z
.array(
z.object({
id: z.string().min(1),
type: z.string().min(1),
props: z.record(z.string(), z.unknown()),
}),
)
.optional(),
});
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SerializedBlockSchema only validates children at depth 1 (lines 12-20), but the Block interface allows for nested children recursively. This creates a mismatch where:

  1. Deeply nested blocks (children with children) won't be validated properly
  2. The schema would accept invalid structures that violate the Block interface

Consider making the schema recursive using z.lazy():

export const SerializedBlockSchema: z.ZodType<Block> = z.lazy(() => z.object({
  id: z.string().min(1),
  type: z.string().min(1),
  props: z.record(z.string(), z.unknown()),
  children: z.array(SerializedBlockSchema).optional(),
}));

This ensures validation works for blocks at any nesting depth.

Suggested change
export const SerializedBlockSchema = z.object({
id: z.string().min(1),
type: z.string().min(1),
props: z.record(z.string(), z.unknown()),
children: z
.array(
z.object({
id: z.string().min(1),
type: z.string().min(1),
props: z.record(z.string(), z.unknown()),
}),
)
.optional(),
});
export const SerializedBlockSchema: z.ZodType<Block> = z.lazy(() =>
z.object({
id: z.string().min(1),
type: z.string().min(1),
props: z.record(z.string(), z.unknown()),
children: z.array(SerializedBlockSchema).optional(),
}),
);

Copilot uses AI. Check for mistakes.
Comment on lines +80 to +94
useEffect(() => {
if (timerRef.current !== undefined) {
clearTimeout(timerRef.current);
}

timerRef.current = setTimeout(() => {
void doSave(blocks);
}, debounceMs);

return () => {
if (timerRef.current !== undefined) {
clearTimeout(timerRef.current);
}
};
}, [blocks, debounceMs, doSave]);
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The useEffect at lines 80-94 has a potentially problematic dependency array. The doSave callback is included in the dependencies, but doSave itself depends on setStatus, which depends on forceUpdate. This creates a complex dependency chain that could cause the effect to re-run more often than intended.

Additionally, the effect will trigger on the initial mount even if blocks is empty or hasn't changed, causing an unnecessary save attempt on first render.

Consider:

  1. Adding a check to skip the initial save (e.g., using a useRef to track first mount)
  2. Using useCallback with more stable dependencies, or
  3. Restructuring to avoid re-creating doSave on every render

Copilot uses AI. Check for mistakes.
Comment on lines +76 to +118
const renderBlock = useCallback(
(block: Block, context: BlockRenderContext) => {
return (
<BlockWrapper
id={block.id}
isSelected={context.isSelected}
isFocused={context.isFocused}
isFirst={context.isFirst}
isLast={context.isLast}
onSelect={(additive) => {
if (additive) {
const next = new Set(selectedIds);
if (next.has(block.id)) {
next.delete(block.id);
} else {
next.add(block.id);
}
setSelectedIds(next);
} else {
setSelectedIds(new Set([block.id]));
}
}}
onFocus={() => setFocusedId(block.id)}
onDelete={() => removeBlock(block.id)}
onDuplicate={() => duplicateBlock(block.id)}
onMoveUp={() => moveBlockUp(block.id)}
onMoveDown={() => moveBlockDown(block.id)}
draggable
>
{renderBlockContent(block, context)}
</BlockWrapper>
);
},
[
selectedIds,
setSelectedIds,
setFocusedId,
removeBlock,
duplicateBlock,
moveBlockUp,
moveBlockDown,
],
);
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The renderBlock callback (lines 76-118) is recreated on every render when any of its dependencies change. Since selectedIds is a Set that gets recreated frequently, and multiple action functions are in the dependency array, this callback will be recreated often.

This is problematic because:

  1. renderBlock is passed to BlockCanvas, likely causing unnecessary re-renders of all blocks
  2. Creating new inline arrow functions for every block event handler on each render is inefficient

Consider:

  1. Memoizing the event handlers separately using useCallback with stable dependencies
  2. Using a context or reducer pattern to avoid passing so many action functions as dependencies
  3. At minimum, ensuring updateBlockProps is included in the dependency array (it's missing but used in the PropertyEditor)

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,58 @@
import type { Block } from "@rafters/ui/components/editor";
import { z } from "zod/v4";
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import pattern import { z } from "zod/v4" is non-standard. The zod package doesn't officially export a /v4 subpath. This import pattern will likely fail at runtime.

The standard import for Zod v4 should be:

import { z } from "zod";

The version is controlled by the package.json dependency, not by the import path. This appears consistently across all block definition files and serialization code, so it needs to be fixed everywhere.

Suggested change
import { z } from "zod/v4";
import { z } from "zod";

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,22 @@
import type { BlockDefinition } from "@rafters/ui/components/editor";
import { z } from "zod/v4";
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import pattern import { z } from "zod/v4" is non-standard. The zod package doesn't officially export a /v4 subpath. This import will fail at runtime.

Use the standard import:

import { z } from "zod";

This same issue appears in all block definition files (divider.ts, embed.ts, form.ts, heading.ts, image.ts, list.ts, paragraph.ts).

Suggested change
import { z } from "zod/v4";
import { z } from "zod";

Copilot uses AI. Check for mistakes.
ssilvius and others added 5 commits January 27, 2026 06:12
The expanded globs (packages/*/src/**, etc.) caused biome to lint
all packages, surfacing pre-existing errors. Revert to original
scope; lint expansion should be done in a separate PR.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add rafters/ui to workspace packages and catalog entries so
the dependency resolves via workspace:* instead of a link: path.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
actions/checkout won't write outside the workspace dir, so check out
rafters into .rafters/ then symlink to ../rafters/ where pnpm expects it.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@ssilvius ssilvius closed this Jan 28, 2026
@ssilvius ssilvius deleted the feat/edit-rebuild branch January 28, 2026 01:42
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