Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions docs/TODO.md
Original file line number Diff line number Diff line change
Expand Up @@ -328,9 +328,8 @@ From SuperOOP analysis and handler chain refactor (PR #54):
Why: v1 hardcodes `"After"`. Users need to control where the node lands.
Exit: mouse Y within target bounding box determines Before/After/Inside; visual indicator shows landing position.

- [ ] **Semantic-aware drop**
Why: text-based splice creates parse errors (e.g. `let id =` with empty RHS after moving the lambda out). Drop should operate at AST level.
Exit: dropping a node produces a syntactically valid result or rejects with an error.
- [x] **Semantic-aware drop** (PR #179)
Done: Before/After replaces source with placeholder via `Renderable::placeholder()`, inserts source text from source map. Exchange unchanged. Playwright E2E tests cover all positions.

- [x] **Unit tests for drag-drop JSON parser and legality**
Why: no test coverage for `StartDrag`/`DragOver`/`Drop` parsing or self-drop/descendant-drop guards.
Expand Down
15 changes: 9 additions & 6 deletions editor/projection_memo.mbt
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,24 @@

///|
/// Returns the cached ProjNode (converted from FlatProj once per invalidation).
/// Safe to call both inside and outside tracked context.
pub fn[T] SyncEditor::get_proj_node(self : SyncEditor[T]) -> @core.ProjNode[T]? {
self.cached_proj_node.get()
self.parser_rt.read(self.cached_proj_node)
}

///|
/// Safe to call both inside and outside tracked context.
pub fn[T] SyncEditor::get_source_map(self : SyncEditor[T]) -> @core.SourceMap {
self.source_map_memo.get()
self.parser_rt.read(self.source_map_memo)
}

///|
/// Returns the full node registry (NodeId → ProjNode).
/// Safe to call both inside and outside tracked context.
pub fn[T] SyncEditor::get_registry(
self : SyncEditor[T],
) -> Map[@core.NodeId, @core.ProjNode[T]] {
self.registry_memo.get()
self.parser_rt.read(self.registry_memo)
}

///|
Expand All @@ -30,21 +33,21 @@ pub fn[T] SyncEditor::get_node(
self : SyncEditor[T],
id : @core.NodeId,
) -> @core.ProjNode[T]? {
self.registry_memo.get().get(id)
self.parser_rt.read(self.registry_memo).get(id)
}

///|
pub fn[T] SyncEditor::node_at_position(
self : SyncEditor[T],
position : Int,
) -> @core.NodeId? {
self.source_map_memo.get().innermost_node_at(position)
self.parser_rt.read(self.source_map_memo).innermost_node_at(position)
}

///|
pub fn[T] SyncEditor::get_node_range(
self : SyncEditor[T],
id : @core.NodeId,
) -> @loom_core.Range? {
self.source_map_memo.get().get_range(id)
self.parser_rt.read(self.source_map_memo).get_range(id)
}
56 changes: 32 additions & 24 deletions editor/sync_editor_tree_edit.mbt
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ pub fn[T] SyncEditor::is_dirty(self : SyncEditor[T]) -> Bool {
/// inside a getter.
pub fn[T] SyncEditor::refresh(self : SyncEditor[T]) -> Unit {
self.projection_dirty = false
let _ = self.cached_proj_node.get()
let _ = self.parser_rt.read(self.cached_proj_node)
}

///|
Expand Down Expand Up @@ -132,14 +132,18 @@ pub fn[T] SyncEditor::apply_text_transform(
}

///|
/// Move a node to a new position in the tree by:
/// 1. Unparsing the source node via T::unparse
/// 2. Deleting the source span (replace with placeholder)
/// 3. Inserting the unparsed text adjacent to the target span
/// Move a node to a new position in the tree.
///
/// Inside (exchange): swaps the expression content of source and target,
/// preserving leading whitespace/separators.
///
/// Before/After: replaces the source expression with its placeholder
/// (keeping the syntax valid) and inserts the source text at the target.
///
/// CAUTION: Insert position must be adjusted when source precedes target
/// in the document (deletion shifts subsequent offsets backward by
/// `source_len - placeholder_len`). This method handles the adjustment.
/// `source_len - placeholder_len`). apply_span_edits handles this via
/// reverse-order application.
pub fn[T : @loom_core.Renderable] SyncEditor::move_node(
self : SyncEditor[T],
source_id : @core.NodeId,
Expand Down Expand Up @@ -190,6 +194,18 @@ pub fn[T : @loom_core.Renderable] SyncEditor::move_node(
)
}
let doc_text = self.get_text()
// Strip leading whitespace from a span to find the expression-only start.
// Source map ranges include leading separators; we keep those in place.
fn skip_ws(text : String, start : Int, end : Int) -> Int {
let mut i = start
while i < end {
match text[i] {
' ' | '\t' | '\n' | '\r' => i = i + 1
_ => break
}
}
i
}
let edits : Array[@core.SpanEdit] = match position {
// Inside = exchange: swap the expression content, preserving leading whitespace
Inside => {
Expand All @@ -202,18 +218,6 @@ pub fn[T : @loom_core.Renderable] SyncEditor::move_node(
),
)
}
// Strip leading whitespace from each span to find the expression-only range.
// Source map ranges include leading separators; we keep those in place.
fn skip_ws(text : String, start : Int, end : Int) -> Int {
let mut i = start
while i < end {
match text[i] {
' ' | '\t' | '\n' | '\r' => i = i + 1
_ => break
}
}
i
}
let src_expr_start = skip_ws(doc_text, src_range.start, src_range.end)
let tgt_expr_start = skip_ws(doc_text, tgt_range.start, tgt_range.end)
let src_expr = doc_text[src_expr_start:src_range.end].to_string()
Expand All @@ -231,20 +235,24 @@ pub fn[T : @loom_core.Renderable] SyncEditor::move_node(
},
]
}
// Before/After = move: remove source and insert at target position
// Before/After = move: replace source with placeholder, insert at target
Before | After => {
let src_text = @loom_core.Renderable::unparse(src_node.kind)
// Extract expression text from source map (whitespace-aware, like Exchange)
let src_expr_start = skip_ws(doc_text, src_range.start, src_range.end)
let src_expr = doc_text[src_expr_start:src_range.end].to_string()
// Replace source expression with placeholder (keeps leading whitespace)
let placeholder = @loom_core.Renderable::placeholder(src_node.kind)
let tgt_insert = match position {
Before => tgt_range.start
_ => tgt_range.end
}
[
{
start: src_range.start,
delete_len: src_range.end - src_range.start,
inserted: "",
start: src_expr_start,
delete_len: src_range.end - src_expr_start,
inserted: placeholder,
},
{ start: tgt_insert, delete_len: 0, inserted: src_text + " " },
{ start: tgt_insert, delete_len: 0, inserted: src_expr + " " },
]
}
}
Expand Down
197 changes: 197 additions & 0 deletions examples/ideal/web/e2e/drag-drop.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
// Drag-and-drop E2E tests for Before/After/Inside (exchange) moves.
// Validates that AST-level operations produce valid syntax (no empty RHS,
// no orphaned separators) by checking the resulting editor text.

import { test, expect, type Page } from '@playwright/test';

// ---------------------------------------------------------------------------
// Helpers
// ---------------------------------------------------------------------------

/** Enter Structure mode and wait for structure blocks to render. */
async function setupStructureMode(page: Page) {
await page.goto('/');
await page.getByRole('button', { name: 'Structure' }).click();
await expect(page.getByLabel('Code editor')).toBeVisible();
await page.waitForFunction(() => {
const ce = document.querySelector('canopy-editor');
return ce?.shadowRoot?.querySelector('.structure-block') !== null;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Make structure-block wait condition actually block

The waitForFunction predicate can return true before the editor is ready: when canopy-editor or shadowRoot is missing, optional chaining yields undefined, and undefined !== null evaluates to true. That means setup may proceed immediately instead of waiting for .structure-block, which can make the new drag/drop tests flaky or mask real regressions depending on render timing. Use a null-safe truthy check (for example != null or an explicit boolean cast) so the wait only passes once a block is present.

Useful? React with 👍 / 👎.

}, { timeout: 10_000 });
}

/** Read the current editor text via the CRDT global. */
async function getEditorText(page: Page): Promise<string> {
return await page.evaluate(() => {
const g = globalThis as any;
if (g.__canopy_crdt && g.__canopy_crdt_handle != null) {
return g.__canopy_crdt.get_text(g.__canopy_crdt_handle) as string;
}
return '';
});
}

/** Count structure blocks of a given type inside the shadow DOM. */
async function countNodes(page: Page, type: string): Promise<number> {
return await page.evaluate(
(type) => {
const ce = document.querySelector('canopy-editor');
if (!ce?.shadowRoot) return 0;
return ce.shadowRoot.querySelectorAll(`.structure-${type}`).length;
},
type,
);
}

/**
* Perform a drag-drop between two structure nodes using dispatched events.
* HTML5 drag-and-drop requires DataTransfer which page.mouse can't provide,
* so we synthesize the full dragstart/dragover/drop/dragend sequence.
*/
async function dragDrop(
page: Page,
srcSelector: string,
srcNth: number,
tgtSelector: string,
tgtNth: number,
position: 'Before' | 'After' | 'Inside',
) {
await page.evaluate(
({ srcSelector, srcNth, tgtSelector, tgtNth, position }) => {
const ce = document.querySelector('canopy-editor');
if (!ce?.shadowRoot) throw new Error('canopy-editor not found');

const srcBlocks = ce.shadowRoot.querySelectorAll(srcSelector);
const tgtBlocks = ce.shadowRoot.querySelectorAll(tgtSelector);
const src = srcBlocks[srcNth] as HTMLElement;
const tgt = tgtBlocks[tgtNth] as HTMLElement;
if (!src || !tgt) throw new Error(`Node not found: src=${srcSelector}[${srcNth}], tgt=${tgtSelector}[${tgtNth}]`);

// Enable draggable (normally done by grip mousedown)
src.draggable = true;

const tgtRect = tgt.getBoundingClientRect();
let clientY: number;
switch (position) {
case 'Before': clientY = tgtRect.top + tgtRect.height * 0.1; break;
case 'After': clientY = tgtRect.top + tgtRect.height * 0.9; break;
case 'Inside': clientY = tgtRect.top + tgtRect.height * 0.5; break;
}
const clientX = tgtRect.left + tgtRect.width / 2;

// Get source nodeId
const srcNodeId = (src as any).__pmNode?.attrs?.nodeId
?? src.getAttribute('data-node-id')
?? '0';

// Create DataTransfer with source node ID
const dt = new DataTransfer();
dt.setData('application/x-canopy-node', String(srcNodeId));
dt.effectAllowed = 'move';

// Dispatch drag sequence
src.dispatchEvent(new DragEvent('dragstart', {
bubbles: true, composed: true, dataTransfer: dt,
}));

tgt.dispatchEvent(new DragEvent('dragover', {
bubbles: true, composed: true, dataTransfer: dt,
clientX, clientY, cancelable: true,
}));

tgt.dispatchEvent(new DragEvent('drop', {
bubbles: true, composed: true, dataTransfer: dt,
clientX, clientY, cancelable: true,
}));

src.dispatchEvent(new DragEvent('dragend', {
bubbles: true, composed: true, dataTransfer: dt,
}));
},
{ srcSelector, srcNth, tgtSelector, tgtNth, position },
);

// Wait for reparse after edit
await page.waitForTimeout(500);
}

// ---------------------------------------------------------------------------
// Tests
// ---------------------------------------------------------------------------

test.describe('Drag-Drop — Before/After/Inside', () => {

test.beforeEach(async ({ page }) => {
await setupStructureMode(page);
// Load the "Basics" example: "let id = \x. { x }\nlet apply = ..."
await page.getByRole('button', { name: 'Basics' }).click();
await page.waitForTimeout(500);
});

test('structure blocks render for default example', async ({ page }) => {
const letDefs = await countNodes(page, 'let_def');
expect(letDefs).toBeGreaterThanOrEqual(2);
});

test('grip element is visible on let_def blocks', async ({ page }) => {
const gripVisible = await page.evaluate(() => {
const ce = document.querySelector('canopy-editor');
if (!ce?.shadowRoot) return false;
const grip = ce.shadowRoot.querySelector('.structure-let_def .structure-grip');
if (!grip) return false;
const rect = grip.getBoundingClientRect();
return rect.width > 0 && rect.height > 0;
});
expect(gripVisible).toBe(true);
});

test('CRDT text is accessible', async ({ page }) => {
const text = await getEditorText(page);
expect(text).toContain('let');
expect(text.length).toBeGreaterThan(10);
});

test('exchange (Inside) swaps two let-definitions', async ({ page }) => {
const textBefore = await getEditorText(page);

await dragDrop(page, '.structure-let_def', 0, '.structure-let_def', 1, 'Inside');

const textAfter = await getEditorText(page);
expect(textAfter).not.toEqual(textBefore);
const letCount = (textAfter.match(/let /g) || []).length;
expect(letCount).toBeGreaterThanOrEqual(2);
});

test('Before drop produces valid syntax with placeholder', async ({ page }) => {
const textBefore = await getEditorText(page);

await dragDrop(page, '.structure-let_def', 1, '.structure-let_def', 0, 'Before');

const textAfter = await getEditorText(page);
expect(textAfter).not.toEqual(textBefore);
// Critical: no "let x = " with empty RHS — placeholder should fill it
expect(textAfter).not.toMatch(/let \w+ = \s*\n/);
const letCount = (textAfter.match(/let /g) || []).length;
expect(letCount).toBeGreaterThanOrEqual(2);
});

test('After drop produces valid syntax with placeholder', async ({ page }) => {
const textBefore = await getEditorText(page);

await dragDrop(page, '.structure-let_def', 0, '.structure-let_def', 1, 'After');

const textAfter = await getEditorText(page);
expect(textAfter).not.toEqual(textBefore);
expect(textAfter).not.toMatch(/let \w+ = \s*\n/);
const letCount = (textAfter.match(/let /g) || []).length;
expect(letCount).toBeGreaterThanOrEqual(2);
});

test('self-drop is rejected (no change)', async ({ page }) => {
const textBefore = await getEditorText(page);

await dragDrop(page, '.structure-let_def', 0, '.structure-let_def', 0, 'Inside');

const textAfter = await getEditorText(page);
expect(textAfter).toEqual(textBefore);
});
});
Loading
Loading