[malibio]

Closes #971

[malibio]

Code Review Summary

Review Type: Initial Review

This PR delivers a well-scoped, coherent set of changes across backend validation, frontend node rendering, and the plugin/store layers. The implementation follows established patterns (TaskNode open button, sharedNodeStore reactivity) and the commit message accurately describes the change surface. The change is a definite net improvement.

Two issues require attention before merge: a backend gap where `handle_create_schema` and `handle_update_schema` never pass `title_template` through to the node properties (meaning the validation added in `behaviors/mod.rs` can never actually trigger from the MCP path), and a template deduplication problem in `create_custom_schema` that silently allows duplicate field entries for repeated tokens.

---

Requirements Check

• ✅ `validate_schema_node()` rejects `title_template` referencing undefined fields with a clear error message
• ❌ MCP `create_schema` and `update_schema` return a proper error on validation failure — see Critical Set up Tauri + Svelte project structure #1 below: neither handler passes `title_template` into the node properties, so validation is never invoked from this path
• ✅ Custom schema types (`isCore: false`) appear in the `/` slash command picker (via schema-plugin-loader auto-registration)
• ✅ Selecting a custom type without `title_template` creates a child node with editable `content`, behaves like a task node
• ✅ Selecting a custom type with `title_template` creates a child node with `content = 'Untitled'`
• ✅ Node with `title_template`: shows raw template string as read-only placeholder when title is empty/Untitled
• ✅ Node with `title_template`: shows interpolated title reactively once properties are filled
• ✅ Node with `title_template`: `content` field is not editable inline
• ✅ Inline node has an "open" button that opens the node in a new tab in the other pane
• ✅ Tests added for new backend validation logic
• ✅ `contentTemplate: 'Untitled'` assertion updated in schema-plugin-loader tests

---

Findings

Critical Issues

[Critical/Blocker] `packages/core/src/mcp/handlers/schema.rs` — `title_template` never reaches the stored node, defeating its own validation

`handle_create_schema` builds the properties JSON at line ~165 without including `titleTemplate`:

```rust
let properties = serde_json::json!({
"isCore": false,
"schemaVersion": 1,
"description": &description_text,
"fields": &namespaced_fields,
"relationships": &relationships
// titleTemplate is absent — it will never be stored or validated
});
```

`CreateSchemaParams` also does not declare a `title_template` / `title_template` field, so even if an MCP caller sends one it is silently dropped. The same problem exists in `handle_update_schema` around line ~508, which rebuilds properties without forwarding `title_template`.

The acceptance criterion says "MCP create_schema and update_schema return a proper error response when validation fails". The new cross-validation in `validate_schema_node()` is correct, but it can only fire when a schema node is constructed with the field present. The MCP path never puts it there, so the validation is unreachable from MCP. The fix is to add `title_template: Option` to `CreateSchemaParams` (and the update params struct) and include it in the `properties` JSON when present.

---

[Critical/Blocker] `packages/core/src/services/node_service.rs` — `create_custom_schema` helper produces duplicate fields for repeated tokens

The new token-extraction loop in the test helper (lines ~10597–10613) does not deduplicate field names. A template like `"{first_name} {first_name}"` produces two `first_name` fields in the `fields` array. This will cause the schema to be rejected by any future uniqueness validation and produces misleading test setups today. The fix is a simple contains-check before pushing:

```rust
if !field_name.is_empty() && !fields.iter().any(|f| f["name"] == field_name) {
fields.push(...);
}
```

While this is a test helper rather than production code, it is shared across multiple tests and incorrect shared helpers are a common source of silent false-positives.

---

Suggested Improvements

[Improvement] `packages/desktop-app/src/lib/design/components/custom-entity-node.svelte` — `BaseNode` event forwarding is duplicated verbatim

The two `` branches in the `{#if hasTitleTemplate}` block are identical except for the `customViewContent` prop. This violates DRY and will require maintaining both copies in sync every time new events are added to BaseNode. A single `` with a conditional `customViewContent` prop would reduce the template by ~25 lines and eliminate the maintenance risk:

```svelte
<BaseNode
{nodeId}
bind:nodeType
bind:content
{children}
customViewContent={hasTitleTemplate && displayContent != null ? titleTemplateSnippet : undefined}
on:createNewNode={forwardEvent('createNewNode')}
...
/>
```

[Improvement] `packages/desktop-app/src/lib/design/components/custom-entity-node.svelte` — `typingTimer` not cleared on component destroy

The `typingTimer` timeout is set via `handleTypingStart()` but never cleared when the component unmounts. In Svelte 5 the correct pattern is `$effect(() => () => { if (typingTimer) clearTimeout(typingTimer); })` to register an on-destroy cleanup. Without it, if the node is removed from the DOM while the timer is pending, the callback fires against a stale `isTyping` reference. This is a minor leak but matches the same pattern TaskNode should follow.

[Improvement] `packages/core/src/behaviors/mod.rs` — cross-validation is order-dependent on the existing syntax pass

The `defined_fields` HashSet is built once before the byte-scanning loop, then the field-name lookup is interleaved inside the loop. This is correct but subtly brittle: the cross-validation only reaches the `defined_fields.contains()` check after the syntax checks pass for that token. If a future developer reorders the early-return conditions inside the loop, they could accidentally skip the cross-check. Consider extracting a small `validate_field_names(template, defined_fields)` helper after the syntax loop completes — separating syntax validation from semantic validation makes both easier to test and modify independently.

---

Nitpicks

• Nit: `packages/desktop-app/src/lib/stores/schemas.ts` — `SIDENAV_CORE_TYPES` is a hardcoded allowlist with a single entry (`'task'`). The inline comment explains the intent well. Consider whether this set belongs in a constants file or configuration rather than the store module as the set grows.

• Nit: `packages/desktop-app/src/lib/design/components/custom-entity-node.svelte` line 122 — `const showEntityHeader = $derived(true)` is always-true. This is dead reactive overhead; a plain `const showEntityHeader = true` suffices (or remove the variable entirely and use the literal in the template).

• Nit: `packages/desktop-app/src/lib/design/components/custom-entity-node.svelte` — the open button label text is the literal string `open` (lowercase). The task-node pattern uses an icon instead. Consider either an icon for visual consistency or `Open` (capitalized) to match button conventions.

---

Recommendation: REQUEST CHANGES

The two critical issues need resolution before merge. The backend gap (MCP never passing `title_template`) means the stated acceptance criterion about MCP returning validation errors is not met, and the deduplication gap in the test helper produces unreliable test scaffolding. The frontend improvements are non-blocking but the duplicated BaseNode block is worth fixing in the same pass since it is cheaply addressed.

[malibio]

Review Addressed

All findings from the code review have been fixed.

✅ 🔴 Critical: title_template unreachable via MCP

Added title_template: Option<String> to both CreateSchemaParams and UpdateSchemaParams. The value is now included as titleTemplate in the properties JSON written to the store. For handle_update_schema (which uses update_node_unchecked and bypasses the behavior pipeline), I added an explicit SchemaNodeBehavior::validate_schema_node() call before saving so cross-validation fires consistently. Updated both MCP tool input schemas to document the parameter.

✅ 🔴 Critical: Duplicate field names in test helper

Added a HashSet in create_custom_schema to deduplicate token names extracted from the template before building the fields array.

✅ 🟡 Important: Duplicate BaseNode blocks

Collapsed the {#if hasTitleTemplate}/{:else} branches into a single <BaseNode> that passes customViewContent conditionally. Eliminates ~20 lines of duplicated event wiring.

✅ 🟡 Important: typingTimer not cleared on destroy

Added onDestroy(() => clearTimeout(typingTimer)) to prevent stale callbacks after unmount.

✅ 🟢 showEntityHeader = $derived(true)

Replaced with a plain const showEntityHeader = true.

⏭️ Skipped: SIDENAV_CORE_TYPES extraction — out of scope, fine as-is for now

[malibio]

Code Review Summary — Re-Review

Review Type: Re-Review (addressing findings from review-20260317-213612)

Three commits landed since the last review tag:

1. 55b5ba1 — Address review: MCP title_template support, dedup fields, cleanup
2. e63a25e — docs(mcp): enrich title_template descriptions with multiple examples
3. 24338928 — test(mcp): add integration tests for title_template handler validation

Both Critical/Blocker issues are resolved. Both Important improvements are resolved. All three commits are well-scoped and cohesive. The test suite added in the third commit is particularly thorough. The change is approved.

---

Verification of Previous Findings

[Critical/Blocker — RESOLVED] title_template never reached the stored node via MCP

CreateSchemaParams and UpdateSchemaParams now each carry title_template: Option<String>. The field is inserted as "titleTemplate" into the properties JSON in both handle_create_schema (line ~178) and handle_update_schema (line ~522). The explicit SchemaNodeBehavior::validate_schema_node() call in handle_update_schema before update_node_unchecked (lines 528–538) ensures cross-validation fires consistently on the update path, which was the core gap. Confirmed correct.

[Critical/Blocker — RESOLVED] Duplicate field names in create_custom_schema test helper

The HashSet guard (seen.insert(field_name.to_string())) is in place. The fix is minimal and correct.

[Improvement — RESOLVED] Duplicate BaseNode blocks

The {#if hasTitleTemplate}/{:else} branches are collapsed into a single <BaseNode> with a conditional customViewContent prop. The result is ~20 fewer lines and a single event-wiring surface to maintain.

[Improvement — RESOLVED] typingTimer not cleared on destroy

onDestroy(() => clearTimeout(typingTimer)) is in place.

[Nit — RESOLVED] showEntityHeader = $derived(true)

Replaced with a plain const showEntityHeader = true.

[Nit — ACKNOWLEDGED / SKIPPED] SIDENAV_CORE_TYPES extraction

The author explicitly noted this as out of scope. Acceptable for now.

---

New Issues in the Three Commits

Suggested Improvements

[Improvement] packages/core/src/mcp/handlers/schema.rs line 512 — Option::or makes title_template irremovable once set

The merge strategy params.title_template.or(schema.title_template) treats None in the request as "keep existing". That is correct for the "omit → preserve" case, but it means there is no way for an MCP caller to explicitly remove a previously set title_template. The only documented workaround is passing an empty string "", which the test at line 237–251 exercises. The comment in that test acknowledges the limitation: "Option with serde default can't distinguish 'omit' from 'clear'".\n\nThis is a real API design gap. As shipped, an AI agent using the MCP tool has no way to express "delete the template" without knowing the empty-string workaround, which is nowhere in the tool description. The tools.rs description for update_schema title_template (line ~1343) says "Set or update the title template" but gives no hint about how to clear it. Given that clearing a template unblocks field removal (the test at line 212 exists precisely for this reason), this is a usage scenario that will be hit in practice.\n\nThe fix is a one-line addition to the update_schema tool description: append ", or pass an empty string to remove the existing template" to the current description text.

[Improvement] packages/core/src/mcp/handlers/schema_test.rs — module is included twice via separate #[cfg(test)] blocks

In schema.rs the file is mounted with:

#[cfg(test)]
#[path = "schema_test.rs"]
mod schema_test;

#[cfg(test)]
mod tests {
    ...
}

The #[path] module directive is correctly scoped to cfg(test), which is fine. However, the two #[cfg(test)] attributes are separate items rather than a single block, which is the conventional pattern. This is minor but note that schema_test uses use super::* while the inline tests module also uses use super::*. They both import from the same parent scope and will compile cleanly. No functional issue — purely a style observation worth keeping in mind if more external test files are added.

Nitpicks

• Nit: packages/core/src/mcp/handlers/schema_test.rs line 17 — let mut store = Arc::new(...) declares store as mut but only the Arc is mutated (via the NodeService::new(&mut store) signature). If NodeService::new takes &mut Arc<SurrealStore> to perform interior mutation, this is correct. It is a slightly unusual signature — a future reader might wonder why an Arc is &mut. No action needed; just noting it may warrant a clarifying comment if the pattern appears often.

• Nit: The create_base_schema helper in schema_test.rs (line ~103) calls .unwrap() on the result without any context message. Given that this is setup code whose failure would obscure the real test intent, a .expect("create_base_schema: handle_create_schema should succeed") would make test failures easier to diagnose.

• Nit: packages/core/src/mcp/handlers/tools.rs — The description string added for title_template in both create_schema and update_schema is very long (single-line strings > 200 chars). This is consistent with the existing style in tools.rs, so no change is needed; flagging only for awareness if a tools-description cleanup pass happens in the future.

---

Final Recommendation: APPROVE

Both blockers are resolved correctly. The improvement commits (doc enrichment, integration test coverage) go beyond what was asked and meaningfully strengthen the feature. The one new Improvement finding (undocumentable title_template clear) is low-severity and easily addressed with a one-line description update — it does not block merge.