Prefer content over cached title in viewer header for non-template types (closes #2005)

[malibio]

Closes #2005

Problem

BaseNodeViewer's unfocused header read title || content, unconditionally preferring the store's cached title for every node type. That cache is only refreshed by a backend round-trip, while optimistic content updates leave it untouched — so a title computed for a transient state survived indefinitely.

The traced repro: typing / fires a normal content update while the node is still typed text; if the debounced persist round-trips before the slash command is selected, the backend legitimately computes strip_markdown("/") → "/". The user then picks "task" and types a real title, but updateNode() patches content only, spreading the stale title forward. Result: the header shows / when unfocused and the correct value the moment it's focused.

Confirmed not a data bug — content and title are both correct and identical in the database.

Fix

Option 2 from the issue. The title-preference is now gated on schemaFormLoader.hasTitleTemplate — the same signal that already governs that input's readonly, placeholder, and oninput/onfocus behavior — so .title wins only where it is genuinely a property-computed value distinct from content. Everywhere else .content is the source of truth, matching the focused and inline (node-row.svelte) render paths.

This matches what the original comment already described as the intent ("Prefer computed title (from title_template)…"); the bug was that it applied to every type rather than only template-driven ones.

Two details worth reviewer attention:

1. No content fallback for template types. For a titleTemplate schema an unresolved title now renders empty, so the read-only header shows its titleTemplate placeholder — consistent with node-row.svelte, which also falls back to the template string rather than to content. The old title || content chain would have leaked raw content in that window.
2. Logic extracted to computeHeaderDisplayValue in node-type-predicates.ts. The established pattern for BaseNodeViewer-adjacent logic is a test harness that mirrors the code, but a mirrored copy can stay green while the component regresses — extracting makes the test exercise the real path.

Acceptance criteria

• Task node header shows current content when unfocused, not a stale cached value
• No regression for titleTemplate-driven types — computed title still displays when unfocused
• Traced sequence verified — see caveat below
• No new test failures (bun run test:all, exit 0)
• bun run quality:fix clean

Testing

Baseline 4439 tests → 4445 (6 new). test:all exit 0: unit 191 files / 4445 tests, browser 24, Rust all green.

Verified the new tests genuinely catch the bug by reverting the helper to the old title || content logic — 3 failures, including the exact / → "Another Task" case — then passing with the fix.

Caveat on criterion 3: the traced sequence is pinned as unit tests against the extracted logic, not driven through the running app. The logic is covered exactly; live UI confirmation was not performed.

Out of scope

The same stale-title preference exists on sibling surfaces reading the same store — notably pluginRegistry.getNodeTitle() (plugin-registry.ts:469, node.title || node.content), which backs computeTabTitle, so the tab title for the reproducing node shows the same stale /. Query list/kanban/table views share the pattern. Left untouched per the issue's Non-Goals; likely worth a follow-up issue.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review — PR #2011

Review Type: Initial Review (verified: 0 prior reviews, 0 comments)

Scope: 3 files, 1 commit (df4ac4c), +71/−12.

---

Summary

A well-targeted, genuinely minimal fix. The diagnosis in the issue is correct and the chosen remedy (Option 2) is the right one: it removes a dependency on a cache being fresh rather than adding machinery to keep that cache fresh, which is the structurally simpler of the two options and eliminates a class of bug rather than one instance. Extracting computeHeaderDisplayValue as a pure function is the right call — the alternative pattern in this codebase (a test harness mirroring component logic) lets tests stay green while the component regresses, and the PR body correctly identifies that.

I independently verified all four claims flagged for scrutiny. Three hold up. One (hasTitleTemplate as a proxy for "backend used a title_template") has a real divergence from the backend's actual rule, but I confirmed it is not reachable today and is not introduced by this PR — details in the findings below.

Net effect on code health is clearly positive. Recommending APPROVE, with one suggestion worth folding in before merge and two follow-ups.

---

Requirements Check (issue #2005)

• ✅ A task node's TaskNodeViewer header shows correct current content when unfocused — task-node-viewer.svelte:30 renders <BaseNodeViewer {nodeId} /> with no header snippet, so it hits the default header input. task is in CORE_NODE_TYPES, so schemaFormLoader.genericSchema is never loaded for it (schema-form-loader.svelte.ts:62-64 gates loadGenericSchema behind isCustomSchemaType), making hasTitleTemplate reliably false and content the source of truth. Fix is correct for the reported case.
• ✅ No regression for titleTemplate-driven types — verified below; the empty-string branch is correct, not a regression.
• ⚠️ Reproduce and verify against the exact traced sequence — pinned at the logic level by unit tests, not driven through the running app. Disclosed in the PR body. Accepting this: the traced sequence's only client-side variable is the (title, content, hasTitleTemplate) triple, and the test at node-type-predicates.test.ts:31-35 reproduces exactly the ('/', 'Another Task', false) state the issue traced. Live UI confirmation would add little given the extracted function is the sole decision point.
• ✅ No new test failures — test:all exit 0, 4439 → 4445.
• ✅ quality:fix clean — svelte-check 0 errors.

---

Findings

🟡 Important: hasTitleTemplate is narrower than the backend's actual title_template rule

📁 packages/desktop-app/src/lib/design/components/schema-form-loader.svelte.ts:32-34, consumed at node-type-predicates.ts:54

This is the highest-risk area, so stating the verification explicitly.

The backend's compute_title() (packages/core/src/services/node_service/crud.rs:2026-2040) takes the template branch for any node type whose schema carries a title_template — it calls get_schema_node(&node.node_type) with no core/custom distinction. The frontend's hasTitleTemplate is strictly narrower: it reads genericSchema?.titleTemplate, and genericSchema is only ever populated for isCustomSchemaType(nodeType) (schema-form-loader.svelte.ts:62-64). So if a core type's schema ever carried a title_template, the backend would compute a property-derived title while the frontend header would show content — displaying the wrong value.

I traced whether that state is reachable and it currently is not:

• create_schema hardcodes "isCore": false (schema/mod.rs:354), so no new schema is core.
• update_schema preserves isCore and can set title_template on an existing core schema (schema/mod.rs:714-731) — the schema_is_core guard at line 605 only rejects unprefixed field names, not title_template. So the API surface permits it.
• But no core type ships a title_template today (the only titleTemplate occurrences in behaviors/mod.rs are test fixtures at lines 3934-4421), and per CLAUDE.md core node types are hardcoded behaviors, not schema-authored.

So: not a bug in this PR, and not newly introduced — the pre-existing code had the same blind spot in the opposite direction (it preferred title for core types even without a template, which is precisely the bug being fixed). This PR narrows the wrong behavior; it doesn't widen it.

Engineering principle — a derived flag should be named for what it actually measures. hasTitleTemplate reads as "this node's title comes from a template," but it means "this node is a custom schema type whose loaded generic schema has a titleTemplate." Those coincide today by accident of which types load a generic schema, not by construction. The risk is that a future change (adding a title_template to a core schema, or loading genericSchema for core types) silently breaks the header with no test catching it.

Recommendation (non-blocking, cheap): add a brief note at the hasTitleTemplate getter recording that it is custom-schema-scoped by construction and that core types are assumed template-free, so the coupling is discoverable when someone changes either side. Not worth restructuring now — YAGNI applies while the state is unreachable.

🟢 Verified correct: no-content-fallback for template types

📁 packages/desktop-app/src/lib/design/components/node-type-predicates.ts:54-56

I checked the claim independently and it holds. node-row.svelte:129-135 computes:

{@const nodeTitleDisplay = nodeHasTitleTemplate
  ? node.title && /\w/.test(node.title) ? node.title : (nodeSlashCmd?.titleTemplate ?? '')
  : undefined}

— for template nodes it goes title → titleTemplate string, never to content. And the header's placeholder path does render the template in that state: base-node-viewer.svelte:1324-1326 sets placeholder={schemaFormLoader.hasTitleTemplate ? (schemaFormLoader.genericSchema?.titleTemplate ?? 'Untitled') : 'Untitled'}. So returning '' surfaces the titleTemplate placeholder, exactly matching inline rendering.

Returning '' is not just acceptable, it's better than the old behavior: the previous title || content chain would leak raw content into a read-only, template-driven header — for a custom entity that content is typically the empty string the slash command inserted (schema-plugin-loader.ts:132, contentTemplate: ''), but where it isn't, it would show a value the user cannot edit and that doesn't match the template. Good catch on that side effect.

One divergence worth noting: node-row.svelte guards with /\w/.test(node.title), so a title that resolved to whitespace or punctuation-only (e.g. template "{first_name} {last_name}" with both fields empty → " ") falls back to the placeholder inline. computeHeaderDisplayValue uses a bare falsy check, so " " is truthy and the header would render a blank-looking string with no placeholder. Cosmetic and narrow, but it's a real inconsistency between two paths the PR explicitly aligns.

🟢 Suggestion: match the inline rule — treat a title with no word characters as unresolved:

const raw = hasTitleTemplate ? (node?.title ?? '') : (node?.content ?? '');
if (hasTitleTemplate && !/\w/.test(raw)) return '';
if (!raw) return '';

Engineering principle: two code paths asserted to be consistent should be consistent at the boundary conditions too, or the asserted invariant quietly isn't one.

🟢 Verified correct: the async hasTitleTemplate window is pre-existing, not new

📁 packages/desktop-app/src/lib/design/components/base-node-viewer.svelte:318-321, 1313-1327

The reasoning in the task description checks out. loadGenericSchema() is async and deliberately not awaited (schema-form-loader.svelte.ts:63 fire-and-forget; line 321 in the viewer likewise), and resetGenericSchema() at line 319 clears it on navigation — so there is genuinely a window where hasTitleTemplate is false for a custom entity.

But every other consumer of that flag on the same input already keys off it identically: class:header-input--readonly (1315), oninput guard (1318), onfocus guard (1320), readonly (1323), placeholder (1324). During that window the input is already momentarily editable and showing the generic 'Untitled' placeholder. Adding the display value to that set doesn't create a new inconsistency — it makes the header's value consistent with the header's other five properties during the same window.

Is there a flash? Yes, briefly — but for a custom entity the content is almost always '' (the slash command inserts empty content), so the flash is empty → template placeholder, not wrong-content → right-content. Pre-existing and benign. The comment at lines 1348-1350 shows the codebase already reasons about this window explicitly for autoOpen.

🟢 Suggestion: module placement for computeHeaderDisplayValue

📁 packages/desktop-app/src/lib/design/components/node-type-predicates.ts:36-57

The concern is fair. The module's own docblock scopes it to node type classification, and this function is header display formatting — it takes a node shape and a boolean, and touches no type taxonomy. Strictly it's a Single Responsibility drift: the module now has two reasons to change.

That said, weighing it pragmatically: the module is small, the function is pure and colocated with its only caller's other import from the same file, and the alternative (a new one-function module, or bundling with formatTabTitle) has its own costs. Notably, formatTabTitle is arguably the better home — it shares the exact replace(/^#+\s*/, '') rule, and that duplication is now in two places. If formatTabTitle lives somewhere reasonable, moving there would consolidate the markdown-stripping rule rather than fork it.

Recommendation: acceptable as-is for merge. If you touch this again, either move it beside formatTabTitle or have both call one shared stripMarkdownHeading() helper, so the stripping regex has one definition. Engineering principle: DRY applies to rules, not just to code — a formatting rule duplicated in two modules will eventually diverge in one of them.

🟢 Test quality: genuinely behavior-pinning, one gap

📁 packages/desktop-app/src/tests/design/node-type-predicates.test.ts:28-70

Not tautological. The tests assert against the old implementation's behavior in the three places the two implementations differ, which is exactly what a regression test should do — and the author verified 3 fail against title || content. The repro test at lines 31-35 encodes the actual traced bug state with a comment explaining why those values, which is the right kind of test comment (intent, not mechanics). Null/undefined handling is covered for both flag values.

Gaps (all minor, none blocking):

1. No test for the whitespace-only template title discussed above — worth adding alongside that fix if you take it.
2. No test that the markdown-stripping applies on the hasTitleTemplate: true path. It currently does (the replace runs after the branch), which is arguably surprising — a template-computed title shouldn't have leading # to strip. Either pin the current behavior or scope the strip to the content branch where it's actually meaningful.
3. Nothing exercises the real Svelte component wiring, so a future edit to base-node-viewer.svelte:349-351 that stops calling the helper (or passes the wrong flag) wouldn't be caught. Accepting this — component-level testing of this viewer is a much larger lift, and extraction already moved the needle substantially versus the mirrored-harness pattern.

---

Other observations

• Blast radius is smaller than it looks ✅ — the concern that this now makes content win for all non-template core types is largely moot in practice: DateNodeViewer supplies its own header snippet (date-node-viewer.svelte:100-128), CollectionNodeViewer builds its own layout with a collection-header (line 274) and doesn't wrap BaseNodeViewer, and QueryNodeViewer uses its own query-header (line 556). Only viewers falling through to the default input are affected. Notably date and schema return Ok(None) from compute_title() outright (crud.rs:2021-2023), so they have no title to prefer anyway — and task/collection use strip_markdown(content), making content the correct source by definition. I found no core type where a backend-computed title legitimately differs from content and reaches the default header.
• Atomicity ✅ — single cohesive purpose, no unrelated changes bundled.
• Standards compliance ✅ — no lint suppression, no any, no console.*, no @deprecated, no backward-compat shims. The old logic is deleted rather than kept behind a flag, consistent with the greenfield policy in CLAUDE.md.
• Documentation ✅ — the docblock on computeHeaderDisplayValue explains why each branch behaves as it does (the staleness mechanism, the placeholder rationale) rather than restating the code. This is the right register for a comment. No ADR needed; no issue numbers embedded in code comments, per the documentation standard.
• Security / performance — nothing applicable. Pure synchronous string function on a $derived; the change from $derived.by to $derived is neutral-to-marginally-better.
• Sibling stale-title surfaces — agreed with leaving pluginRegistry.getNodeTitle() (plugin-registry.ts:469, node.title || node.content) untouched per the issue's Non-Goals. Worth flagging that this means the tab title for the reproducing node still shows / after this fix, so the user-visible bug is only partially resolved. The PR body discloses this. Please do open the follow-up issue — a fix that resolves the header but leaves the tab stale is the kind of thing that gets rediscovered as a "new" bug in three weeks.

---

Recommendation: APPROVE

Clear net improvement to code health. The fix is correctly diagnosed, minimally scoped, and removes a fragile dependency rather than papering over it. All four flagged risk areas verified independently — the three claims hold, and the fourth (hasTitleTemplate vs. the backend's broader rule) is a real but currently-unreachable divergence that predates this PR.

Nothing here blocks merge. The whitespace-only-title consistency fix is the one item I'd suggest folding in first since it's a two-line change to the invariant this PR is explicitly asserting; the module-placement and tab-title items are follow-ups.

[malibio]

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

[malibio]

Address-Review Summary

Commit f72717b. Review verdict was APPROVE with nothing blocking; this folds in the suggested consistency fix and the related test gap.

✅ Addressed (3)

1. Whitespace-only template title (the reviewer's "fold in first" item)
computeHeaderDisplayValue now treats a title with no word characters as unresolved, matching the /\w/ guard node-row.svelte:129-133 applies when rendering these nodes inline. Previously a title like " " — a "{first_name} {last_name}" template with both fields empty — was truthy, so the header rendered a blank-looking value with no placeholder. That broke the exact header/inline invariant this PR asserts.

2. Markdown-strip scoped to the content branch (test gap 2)
The strip previously ran after the branch, so it applied to computed titles too. A template-built title is assembled from property values, so a leading # is literal text rather than header syntax. Pinned with "#1 Priority Account" surviving intact.

3. hasTitleTemplate discoverability note (🟡 Important)
Documented at the getter that it is custom-schema-scoped by construction, while the backend's compute_title() applies a title_template to any schema carrying one, core or not — and that the two agree only because no core type ships a template today. Records the coupling so changing either side is discoverable, without restructuring while the divergent state is unreachable.

Also corrected an inaccuracy in my own docstring found while verifying: it claimed the strip was the "same rule as formatTabTitle", but that helper additionally takes the first line, trims, and truncates. Only the regex is shared.

⏭️ Skipped (2)

Moving computeHeaderDisplayValue beside formatTabTitle / extracting a shared stripMarkdownHeading() — the review marked placement acceptable as-is and conditioned the move on touching the code again. Doing it now widens the diff past the bug fix for no behavior change. The DRY point is valid and worth taking when either stripping site is next edited.

Component-level test of the base-node-viewer wiring — explicitly accepted by the reviewer as a much larger lift; the extraction already removed the mirrored-harness failure mode that motivated the concern.

🧪 Verification

• bun run test:all exit 0 — unit 191 files / 4447 tests (4445 → 4447, +2 new), browser 24, Rust all crates green
• bun run quality:fix clean — 0 errors, 0 warnings
• Verified both new tests fail against the prior helper and pass with it, so they pin real behavior changes rather than restating the implementation

📋 Follow-up filed

#2012 — the sibling stale-title surfaces (pluginRegistry.getNodeTitle() at plugin-registry.ts:469 backing computeTabTitle, plus query list/kanban/table views and collections.svelte.ts). As the review noted, the tab title for the reproducing node still shows / after this PR, so the user-visible bug is only partially resolved.

Re-Review Decision

NO RE-REVIEW NEEDED. Three narrow edits to a single pure function plus two comments — no architectural change, no new functionality, no refactoring. Each behavior change is test-pinned against the prior version, the full suite is green, and both skips were pre-authorized by the review itself.