Add indexed title field for efficient keyword search in @mention system

[malibio]

Closes #821

[malibio]

Code Review Summary

Review Type: Initial Review
PR: #823 - Add indexed title field for efficient keyword search in @mention system
Branch: feature/issue-821-indexed-title-field -> main

---

Overall Assessment

This is a well-designed implementation that adds an indexed title field to enable efficient @mention autocomplete search. The approach correctly addresses the SurrealDB limitation (no partial indexes) by using a separate field that's only populated for searchable nodes. The code is clean, well-documented, and follows established project patterns.

Recommendation: APPROVE

The changes represent a clear net improvement to the codebase with proper architecture, good test coverage, and clear documentation of future refactoring (TODO #824).

---

Requirements Validation

From Issue #821:

Phase 1: Schema & Backend

• Add title field to SurrealDB node schema - schema.surql:40
• Create full-text search index on title field - schema.surql:49 (Note: uses standard index, comment explains why)
• Update NodeService::create_node to populate title for root/task nodes - node_service.rs:1168-1182, 1329-1357
• Update NodeService::update_node to sync title when content changes - node_service.rs:2100-2134
• Handle edge case: node type changes to/from task - node_service.rs:2109-2116, 2280-2310

Phase 2: Query Integration

• Update @ mention autocomplete to search against title - surreal_store.rs:2416-2473
• Simplified query eliminates expensive relationship traversal - Excellent performance improvement

Phase 3: Migration & Testing

• Create migration to populate title for existing root/task nodes - Not implemented (acceptable for pre-release)
• Add unit tests for title population/sync logic - Updated existing tests in surreal_store.rs
• Add integration tests for mention search using title - 6 mention_autocomplete tests pass
• Verify performance improvement with indexed search - Query simplified from expensive relationship traversal to indexed title search

Phase 4: Quality

• Code passes bun run quality:fix - Verified
• All tests pass - 706 Rust tests passing

---

Code Review Findings

Critical Issues

None identified.

Suggested Improvements

1. [Improvement] packages/core/src/utils/markdown.rs:92 - Minor performance consideration

   The strip_markdown function compiles a new regex for whitespace cleanup on every call:

   let whitespace_re = Regex::new(r"\s+").unwrap();

   Consider adding this to the MARKDOWN_PATTERNS LazyLock or creating a separate LazyLock for consistency with the rest of the function. This is a micro-optimization and not blocking given the likely low frequency of calls.

2. [Improvement] Missing migration for existing data

   The acceptance criteria mentions "Create migration to populate title for existing root/task nodes". While not critical for pre-release (as documented in CLAUDE.md), existing nodes in development databases won't appear in @mention search until recreated or manually updated.

   Consider adding a one-time migration script or documented SQL snippet in the PR description for developers to run.

Nitpicks

1. Nit: packages/core/src/services/node_service.rs:2114 - The get_parent async call inside should_have_title logic adds an extra database query for non-task, non-excluded types. This is acceptable since most searchable nodes will be tasks or excluded types, but worth noting for awareness.

2. Nit: packages/core/src/db/surreal_store.rs:49 - The comment mentions BM25 analyzer but the actual index is a standard index. The comment correctly explains why, but could be slightly clearer that BM25 was the original intent but not currently used.

3. Nit: packages/core/src/models/node.rs:610 - The double-Option pattern Option<Option<String>> for title updates is a valid Rust idiom but consider adding a brief doc comment explaining the semantics (Some(Some(x)) = set, Some(None) = clear, None = no change).

---

Architecture & Design Analysis

Strengths:

1. Clean separation of concerns - The strip_markdown utility is properly extracted to utils/markdown.rs with comprehensive tests
2. Future-proof design - TODO comments (Add title_template support for schema-driven title computation #824) mark hardcoded exclusions for schema-driven refactoring
3. Performance improvement - The new query eliminates expensive count(<-relationship[...]) traversal
4. Defensive programming - Title updates properly handle type changes (task -> non-task) by clearing titles

Trade-offs (Acceptable):

1. Using standard index instead of full-text BM25 - SurrealDB configuration limitation acknowledged
2. Hardcoded type exclusions (date, schema, collection) - Tracked for future schema-driven approach in Add title_template support for schema-driven title computation #824

---

Security Analysis

• No security concerns identified
• No user input handling changes that could introduce vulnerabilities
• The strip_markdown function safely handles content without executing it

---

Test Coverage Assessment

• Unit tests: Comprehensive strip_markdown tests (14 test cases covering headers, links, images, code, etc.)
• Integration tests: 6 mention_autocomplete tests updated and passing
• Edge cases covered: Type exclusions, nested vs root nodes, case insensitivity, limit handling

---

Final Notes

This PR demonstrates good engineering practices:

• Clear commit messages explaining the "why"
• Proper TODO tracking for future refactoring (Add title_template support for schema-driven title computation #824)
• Tests updated to reflect new behavior
• Documentation in code explaining design decisions

The implementation correctly addresses the core problem (efficient @mention search) while setting up for future improvements (schema-driven title templates).

---

Reviewed by: Principal Engineer Reviewer (Pragmatic Quality Framework)

[malibio]

Review Recommendations Addressed

Summary:

• ✅ Addressed: 1 recommendation
• ⏭️ Skipped: 4 recommendations (with justification)

Implemented Changes

🟢 Suggestion: Static regex compilation (markdown.rs:92)

• Moved whitespace regex to static LazyLock for consistency with MARKDOWN_PATTERNS
• Commit: f60a2ae

Skipped Recommendations (with Justification)

1. Doc comment for double-Option pattern - ✅ Already implemented

   • Lines 380-381 in node.rs already contain the doc comment: "Use Some(Some(title)) to set a title, Some(None) to clear it"

2. Extra get_parent query - ⏭️ Acceptable tradeoff

   • Reviewer acknowledged this is acceptable since most searchable nodes are tasks or excluded types

3. BM25 comment clarity - ⏭️ Minimal value

   • Current comment already explains why standard index is used instead of BM25
   • Additional clarification provides negligible benefit

4. Missing migration script - ⏭️ Per project guidelines

   • CLAUDE.md explicitly allows skipping migrations in pre-release (no users, no production data)
   • Developers can recreate nodes or run manual SQL if needed

Tests

• ✅ All markdown tests pass (57 tests)
• ✅ Quality checks pass (eslint, svelte-check, clippy)

---

Addressed by: Implementation Agent (Address Review Framework)