[malibio]

Closes #3512

What changed

• Import: both import paths (bulk and single-file) create the document root as a text node (CoreNodeType::Text) whose content is the plain title. A title taken from the file's first line loses a leading #…###### heading marker; a filename-derived title is used as is. Re-import and --replace address the same deterministic root id; replace updates the root's content.
• Collection member list: the type icon before each member row is removed. The row shows the name and the type tag. getNodeIcon stays, since the "Add existing" search results still use it.

Notes

• A root imported before this change keeps its header type; --replace updates its content only. Re-import into a reset database to get the new shape.

Testing

• bun run test:changed passed (frontend, scripts, browser, drift checks, Rust workspace).
• Import tests assert root type and content on both paths; new unit test for title stripping; new component test for the icon-free member row.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review — PR #3531

Review Type: Initial Review (no prior reviews or comments on the PR; one commit, f135149c)

Summary

The change does what the issue asks, in the smallest reasonable diff: one helper (root_content_for_title) feeds both import paths, the root type comes from CoreNodeType::Text rather than a literal, and the member-row icon is a one-line removal. No compatibility code was added for existing header roots, which is what the pre-release rule requires. One doc comment landed in the wrong place and should be fixed before merge; the rest is optional.

I read the diff and the surrounding code. I did not run any tests (per the brief; bun run test:changed is reported as passing on this branch).

Requirements Check

	Criterion	Notes
✅	Bulk import creates each root as a text node with the plain title	prepare_file_import builds root_content through the helper; write_chunk inserts the root row with CoreNodeType::Text.as_str(). The batch test asserts type and content for both documents.
✅	Single-file import does the same	import_single_file computes root_content and import_markdown_content creates the root as CoreNodeType::Text. The single-file test asserts type and content.
✅	# Title, ## Title, Title all yield Title; filename title unchanged	root_content_is_the_plain_title covers levels 1, 2 and 6, a plain line, extra whitespace, #tag, seven #, and a filename title that starts with # .
✅	The root's title (search, collection list, nav) is the plain title	Follows from derive_title, which returns strip_markdown(content) for any root. Not asserted by a test in this PR; the content assertions cover the input to it.
✅	Re-import, with and without --replace, addresses the same root, no duplicate	deterministic_root_id and the exists branches are untouched. Both replace paths write the new plain content. Tests assert the root id, type and content after replace.
✅	Member list shows no icon; name and type tag unchanged; rows aligned	Icon removed from .node-item. No CSS change is needed: gap only applies between flex children, so the name now starts at the row's 1rem padding. The "Add existing" results keep their icon, so getNodeIcon correctly stays. The new component test would have failed before the change (Icon renders an <svg>).
✅	Import tests that count header nodes or assert root type are updated	All five count_nodes_by_type("header") call sites moved to "text"; root type assertions added. See the suggestion below on what the count now covers.
➖	bun run quality:fix / bun run test:changed	Not re-run by this review.

Code Review Findings

🟡 Important

1. The new helper was inserted between deterministic_root_id and its doc comment
📁 packages/daemon/src/services/import_service.rs:67-87

The three lines "Derive a document's root node id deterministically from its identity key…" now sit directly above the helper's own comment, so rustdoc attaches both paragraphs to root_content_for_title, and deterministic_root_id (line 85) has no documentation. The orphaned comment explains why the id is keyed on the base-directory-relative path, which is the property re-import depends on.

Recommendation: move root_content_for_title and its comment below deterministic_root_id, or above line 67, so each function keeps its own comment.

Principle: comments that explain "why" must sit on the code they explain; a comment on the wrong function is worse than none.

🟢 Suggestions

2. import_markdown_content now takes both title and root_content
📁 packages/daemon/src/services/import_service.rs:1424-1432, call site 248-258

title is now used only to decide whether the first line is dropped from the children, and root_content is derived from it one line before the call. Two strings that must agree are passed separately, and every test caller has to keep them in sync by hand ("# Setup", "Setup"). Passing title plus the use_filename_as_title flag and calling the helper inside the function would leave one source of truth. Optional; the current shape is correct.

Principle: DRY / make inconsistent states unrepresentable.

3. The "no duplicate" assertions now count only text nodes
📁 packages/daemon/src/services/import_service.rs:2123, 2133, 2165, 2268, 2281, 2296, 2366, 2380

Before, the header count covered the root and every section heading. It now covers the root and paragraphs, and no longer sees a duplicated section heading, while the messages still say "must not duplicate any node". The tests remain valid for the root. If a total node count is available on the store, it matches the message better; otherwise leave it.

Principle: an assertion should cover what its message claims.

Nitpicks

• Nit: packages/daemon/src/services/import_service.rs:77-82 — a first line that is only a marker (# or # ) gives the root content #, and #<tab>Title is not treated as a heading although CommonMark allows a tab. Both are rare and neither is a regression in practice; a test line documenting the chosen behaviour would be enough.

Checked and found fine

• No node_type == "<literal>" comparison in product code; the tests use CoreNodeType::Text.is_exactly.
• No migration or compatibility code for roots imported earlier as header; the PR notes say to reset, as CLAUDE.md requires.
• The byte slice in the helper is safe on non-ASCII titles: level counts only ASCII # bytes, so trimmed[level..] is always on a character boundary.
• No issue numbers in code comments. No security or performance impact. Nothing under nodespace-docs or the skill describes an imported root as a header, so no doc change is needed.
• Out of scope, for awareness only: the markdown parser's own first-line root (packages/core/src/markdown/mod.rs, used outside import_service.rs) still produces a header root. The issue limits itself to the two import-service paths.

Recommendation

APPROVE, with finding 1 worth fixing before merge. Findings 2, 3 and the nit are optional.

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Review addressed

Addressed (3)

• Finding 1: root_content_for_title moved below deterministic_root_id, so each doc comment sits on its own function.
• Finding 2: import_markdown_content takes the title and whether it came from the filename, and derives the root content itself.
• Finding 4: the title test now documents marker-only (# ) and tab-separated first lines.

Skipped (1)

• Finding 3 (count all nodes instead of text nodes): the same tests also compare subtree sizes and root ids, which cover a duplicated heading.

Daemon import tests: 18 of 18 pass. quality:fix clean.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED
Rationale: a function move, a parameter swap with no behaviour change, and two added test lines.