[malibio]

Closes #1564

[malibio]

Pragmatic Code Review — PR #1565

Review Type: Initial Review

Requirements Check (issue #1564)

• ✅ Tab title always matches the date shown in the content pane after navigation — updateTabContentAndTitle() (navigation.ts:134-143) writes content and title in one tabState.update() call; date-node-viewer.svelte's navigateDate() now calls a single onNavigate(nodeId, title) instead of two separate callbacks. This closes the desync window described in the issue. Verified with a dedicated regression test (navigation.test.ts:347-369) that subscribes mid-update and asserts no mismatched intermediate state is observable.
• ✅ No unhandled promise rejection from hydrateNode — ensureNode() await is now wrapped in try/catch (pane-content.svelte:87-93), logs and returns instead of throwing.
• ✅ Date navigation can reliably land on any target date, including via repeated/rapid prev/next clicks — the latestRequestedNodeId guard (pane-content.svelte:80, 85, 97) discards a stale ensureNode resolution that resolves after a newer navigation superseded it, so out-of-order network responses can no longer overwrite hydratedNodeIds for the wrong date. Traced this against the $effect at lines 138-148: the effect reruns whenever activeTab.content.nodeId changes, including a return to a previously-requested nodeId, so there is no permanent-stuck-hydration failure mode — worst case is a harmless duplicate in-flight ensureNode call for the same id.
• ✅ No state_unsafe_mutation console error during date navigation — plausible given the guard prevents the specific "resume mutates $state for a nodeId that's no longer current" pattern the issue described, and this reviewer could not construct a residual repro from the diff. Not independently reproduced in a live app session (no browser-driven verification was performed as part of this review — see Testing below).
• ✅ App remains responsive to node clicks after date navigation — plausible as a downstream fix, since an uncaught rejection during a reactive flush is a documented way to corrupt Svelte's scheduler; the try/catch removes that trigger. Not independently verified end-to-end.
• ⚠️ Typing / in a node's editing area no longer causes the node to disappear — Not directly addressed by this diff. Nothing in this PR touches editor or slash-command code (text-node.svelte, slash-command-dropdown.svelte, etc.). The issue frames this as one symptom of the same reactivity-corruption cascade, which is a reasonable hypothesis, but this PR provides no test or explicit trace connecting the fix to this specific symptom. If this symptom is still reproducible after merge, the issue should stay open or a follow-up should verify it explicitly — closing Date navigation triggers state_unsafe_mutation and leaves the app unresponsive #1564 on this PR alone assumes a causal link that isn't demonstrated.
• ✅ Regression test added covering rapid date navigation and a simulated slow/failed ensureNode fetch — pane-content-hydration.test.ts covers exactly this (out-of-order resolution, failure, not-found-after-superseded, and a 5-way rapid-navigation convergence test).
• ✅ Code passes bun run quality:fix — confirmed clean (eslint on changed files ran with no output; author reports full quality:fix, cargo fmt/clippy clean).
• ✅ No new failures in bun run test:all — author-reported 148→149 files, 3952→3959 tests, all passing; this reviewer independently re-ran the two directly-relevant test files (72 tests) and confirmed a clean pass.

Code Review Findings

🟡 Important: "Typing /" and "app unresponsive" acceptance criteria are unverified, not just unaddressed

📁 Issue #1564 acceptance criteria; no corresponding code/tests in this diff

The fork verification confirms no commit besides this PR's references #1564, and no test in this PR exercises slash-command input or post-navigation click-responsiveness. The fix is well-targeted at the mechanism the issue's root-cause analysis describes (async $state mutation resuming during a reactive flush, uncaught rejection corrupting the scheduler), and it's plausible all four symptoms share that one root cause — but "plausible" isn't "verified." Recommend either: (a) manually reproducing the original repro steps (type /, navigate rapidly, then click nodes) against this branch before merge and reporting the result in the PR, or (b) leaving #1564 open / filing a fast-follow issue if the slash-command symptom persists, rather than silently assuming it's fixed. This is a process gap, not a code defect — flagging because acceptance criteria should be demonstrated, not inferred.

🟡 Important: The atomic-update pattern isn't enforced against regression

📁 packages/desktop-app/src/lib/types/node-viewers.ts:15-19, packages/desktop-app/src/lib/components/layout/pane-content.svelte:181-191

onNodeIdChange and onTitleChange remain live, independently-callable props in the shared NodeViewerProps interface, wired separately in pane-content.svelte (lines 181-184) alongside the new onNavigate. Today only date-node-viewer.svelte performs combined nodeId+title navigation, and it has been migrated — so there's currently no second path back to the bug. But nothing (type-level, lint rule, or code comment enforcement) stops a future viewer from calling both onNodeIdChange and onTitleChange for one logical navigation and silently reintroducing the exact race this PR fixes. Given this is a known Svelte-reactivity footgun class (the issue explicitly calls it out as "a known Svelte 5 footgun"), consider strengthening the JSDoc on onNodeIdChange/onTitleChange in node-viewers.ts to explicitly warn "do not call together with a title change for the same logical navigation — use onNavigate instead," so the guidance survives beyond this PR's tribal knowledge. Nit-adjacent but flagged as Important because the failure mode is the literal bug this PR fixes, not a generic style concern.

🟢 Suggestion: hydratedNodeIds grows unboundedly for the lifetime of a pane

📁 packages/desktop-app/src/lib/components/layout/pane-content.svelte:42, 105

Pre-existing behavior (not introduced by this PR) — every nodeId ever hydrated in a pane's lifetime stays in the Set forever, with no eviction on tab close. For a long daily-journal session with heavy date navigation this is an unbounded-growth memory characteristic, though in practice bounded by realistic session length (a Set<string> of ISO dates is cheap). Not a blocker; worth a follow-up if profiling ever shows it matters.

🟢 Suggestion: Duplicate in-flight ensureNode calls possible on nodeId revisit

📁 packages/desktop-app/src/lib/components/layout/pane-content.svelte:82-83, 138-148

Confirmed via trace: switching pane tabs A→B→A before A's original ensureNode resolves causes the $effect to refire hydrateNode for A a second time (since hydratedNodeIds.has(nodeId) is still false), issuing a second concurrent fetch for the same nodeId. Harmless (idempotent, guarded by the same latestRequestedNodeId check on each resolution) but wasteful. Not worth blocking on given ensureNode is presumably cheap/cached in the happy path, but worth a comment or dedupe if this pattern becomes hot.

Nit: Test file re-implements rather than exercises the real component

📁 packages/desktop-app/src/tests/components/pane-content-hydration.test.ts:1-20

The test harness (createHydrationHarness) is a hand-copied reimplementation of hydrateNode's logic, not an import of or drive-through the actual pane-content.svelte component. This is a legitimate tradeoff given the documented constraint (full BaseNodeViewer-tree mounts require complex NodeServiceContext/plugin-registry/DB mocking, per the established merge-prevention.test.ts precedent cited in the file's own doc comment) — and the tests do exercise real async/microtask timing rather than mocking it away, which is the part that actually matters for a race-condition regression test. The residual risk is standard for this pattern: if a future edit changes the real hydrateNode (e.g., changes the order of the hydratedNodeIds.has() check vs. latestRequestedNodeId assignment, or removes the stale-discard check), this test suite will keep passing while the real component regresses, because nothing keeps the two copies in sync. Given the codebase's established precedent for this tradeoff, this is a nit rather than a blocker — but if this race-guard pattern gets reused for a third call site, it's worth extracting hydrateNode's guard logic into a small shared/testable helper that both the component and the test import, rather than continuing to hand-copy it.

Testing Assessment

Test coverage for the specific mechanism being fixed (store atomicity, hydration race guard under out-of-order async resolution, failure handling) is thorough and well-targeted — the four pane-content-hydration.test.ts cases specifically hit the out-of-order-resolution scenario described in the issue, not just happy-path coverage. The navigation.test.ts additions correctly verify the atomic write and non-interference with other tabs. The gap is coverage of the two acceptance criteria not mechanically traceable to this diff (slash-command disappearance, click-responsiveness) — see Important finding above.

Recommendation: COMMENT

This is a well-scoped, net-positive fix for the mechanism the issue's root-cause analysis identifies: it correctly closes the title/content desync window with an atomic store write, and correctly guards against stale async hydration responses racing state mutation, with test coverage that specifically targets the race rather than just the happy path. Code quality, patterns, and test rigor are solid, and the two probably-related symptoms (state_unsafe_mutation, unhandled rejection) are directly addressed at the code level.

However, two of six user-facing acceptance criteria in #1564 — the disappearing node on / and app-unresponsive-after-navigation — are not directly demonstrated as fixed by anything in this diff; they're inferred via a shared-root-cause hypothesis that is reasonable but untested. Recommend a manual verification pass against the original repro steps before merge (or explicit written confirmation in the PR that this was done), rather than blocking the PR outright — the fix as written is a genuine improvement regardless of whether it fully resolves every symptom, and if the untested symptoms turn out to persist, that's better discovered now than after close-as-fixed.

[malibio]

Comment-only review posted above: solid, well-tested fix for the atomicity/race-guard mechanism, but 2 of 6 acceptance criteria (slash-command node disappearance, post-navigation click-responsiveness) are unverified rather than demonstrated by this diff. Recommend a manual repro check before merge; not blocking.

[malibio]

Pragmatic Code Review — PR #1565 (Re-review of commit a4b3ced)

Review Type: Re-review of new commit since prior review (314b709 → a4b3ced). Scope limited to what changed in this commit; not re-litigating findings already resolved or unrelated to it.

Prior Review Findings — Disposition

• (a) Two acceptance criteria (slash-command disappearance, click-responsiveness) unverified — Out of scope for this commit; unaffected. Per the task brief, live manual verification was reportedly done separately. Still worth having that confirmation written into the PR description before merge, but this commit doesn't change the analysis.
• (b) onNodeIdChange/onTitleChange coexisting with onNavigate, no guardrail against reintroducing the race — RESOLVED, and resolved more thoroughly than the suggested fix. The prior review's suggestion was to strengthen JSDoc as a guardrail. This commit goes further: it deletes the entire push-based title mechanism. onTitleChange, onNavigate, and disableTitleUpdates are removed from NodeViewerProps (packages/desktop-app/src/lib/types/node-viewers.ts) and from every call site — pane-content.svelte, base-node-viewer.svelte, date-node-viewer.svelte, ai-chat-node-viewer.svelte, collection-node-viewer.svelte, query-node-viewer.svelte, task-node-viewer.svelte. Verified via repo-wide grep: zero remaining references to onTitleChange, onNavigate, disableTitleUpdates, updateTabTitle, updateTabContentAndTitle anywhere in packages/desktop-app/src (one doc-comment mention in node-viewers.ts explaining why the callback was deliberately removed). There is no longer a second path back to the bug because there is no push path left at all — a stronger guarantee than a lint/JSDoc guardrail, since the vulnerable pattern is structurally impossible now, not just discouraged.
• (c) hydratedNodeIds unbounded growth / duplicate in-flight fetches — Untouched by this commit (pane-content.svelte's diff here is a pure 15-line deletion of the onTitleChange/onNavigate prop wiring; the hydration guard logic is unchanged). Correctly out of scope — still valid as a low-priority follow-up, not this commit's concern.
• (d) Test file reimplements rather than exercises real component — Applies to pane-content-hydration.test.ts, untouched by this commit. Not re-litigated. (See new finding below, which is the same category of concern applied to the new mechanism.)

Architectural Assessment of the New Commit

This is a legitimate root-cause fix, not just closing the prior review's comment. The prior commit made the symptom (title/content desync) atomic via updateTabContentAndTitle. This commit removes the underlying pattern that caused state_unsafe_mutation in the first place: pushing a computed title into a store as a side effect of render/mount (base-node-viewer.svelte's old $effect calling untrack(() => updateTabTitle(...)), date-node-viewer.svelte's onMount push, and three other viewers' effect-driven pushes). Titles are now purely derived at read time via computeTabTitle() → pluginRegistry.getNodeTitle(node), reading reactively from sharedNodeStore (confirmed backed by SvelteMap, per-key reactive tracking). This is the textbook Svelte 5 fix for this class of bug — replace "effect pushes derived state into a store" with "compute it where it's consumed" — and it eliminates an entire category of footgun rather than papering over one instance.

Verified claims from the task brief, all confirmed by reading the diff directly:

• onTitleChange/onNavigate/disableTitleUpdates fully removed from NodeViewerProps and every implementing viewer — confirmed via diff + grep, no stragglers.
• Date node title path: dateNodePlugin.getTitle (core-plugins.ts:279-283) parses node.id via parseDateString and formats via formatDateTitle, exactly mirroring the old date-node-viewer.svelte onMount/navigateDate logic that computed title from nodeId, not content. Correct 1:1 behavioral migration.
• Generic/schema nodes with title_template: pluginRegistry.getNodeTitle() falls back to node.title || node.content when a plugin doesn't define getTitle (confirmed: only dateNodePlugin defines it; task/text/query/ai-chat/collection plugins don't). This textually matches the removed base-node-viewer.svelte comment "Prefer computed title (from title_template) over raw content when available" — the assumption that node.title is backend-computed for schema types is supported by that comment being preserved verbatim in the new fallback path's intent, though it's worth noting neither the old nor new code has an inline comment in plugin-registry.ts itself explaining that node.title originates from a backend title_template computation — the connective tissue lives only in the deleted base-node-viewer.svelte comment and the PR's own framing. Minor documentation continuity gap, not a functional one.
• tab-persistence-service.ts migration removal: confirmed correct. The deleted migrate() logic recomputed date titles from persisted nodeId on load specifically because titles used to be persisted, stale, static values. Now that tab-system.svelte calls computeTabTitle(tab, ...) live in the render path (confirmed: {@const title = computeTabTitle(tab, (nodeId) => sharedNodeStore.getNode(nodeId))} at the {#each} level, used for the DOM title attribute, visible tab label, and close-button aria-label), a persisted tab.title value is never read as the displayed title for node tabs — it's only a stale fallback for non-node tabs or nodes not yet in sharedNodeStore. The corresponding 5 test cases in tab-persistence-service.test.ts's "date node title migration" describe block were correctly deleted alongside the deleted migrate() logic they tested.
• No breaking-change fallout found outside the diff: grepped all of packages/desktop-app/src for onTitleChange, onNavigate, disableTitleUpdates — zero hits outside one explanatory doc comment. No other package in the workspace references these (plugin registration is desktop-app-internal; no external plugin API surface exists yet to break).

Findings

Suggested Improvements

• computeTabTitle() has no direct unit test. 📁 packages/desktop-app/src/lib/utils/tab-title.ts, packages/desktop-app/src/tests/plugins/plugin-registry.test.ts. The new "getNodeTitle (issue Date navigation triggers state_unsafe_mutation and leaves the app unresponsive #1564)" describe block (5 tests) thoroughly covers pluginRegistry.getNodeTitle(), but computeTabTitle() itself — which adds its own logic on top (the tab.type !== 'node' / !tab.content early-return, the !node fallback, and the formatTabTitle() wrapping) — has zero direct test coverage, and there is no tab-system.svelte component test at all (confirmed: none exists before or after this commit) to cover it indirectly through rendering either. This is the same category of gap the prior review flagged as a nit for pane-content-hydration.test.ts (test coverage sits one layer away from the code path that actually ships), but here it's slightly more consequential because computeTabTitle's own branches (non-node tabs, missing node, empty title fallback) are entirely unexercised — only the plugin-registry layer it calls into is tested. A handful of direct unit tests for computeTabTitle (mock Tab + a stub getNode) would close this cheaply given the function is a small, pure, five-line dispatcher — this is a good target for a quick top-up rather than a blocking gap, since the underlying getNodeTitle logic it delegates to is well-covered.

• The node.title backend-computation assumption is asserted only in a deleted comment, not the new code path. 📁 packages/desktop-app/src/lib/plugins/plugin-registry.ts:458-467. getNodeTitle()'s fallback node.title || node.content silently relies on the invariant that node.title is populated by the backend for schema types with a title_template (this was stated explicitly in the now-deleted base-node-viewer.svelte comment: "Prefer computed title (from title_template) over raw content when available"). Consider carrying that one-line rationale into getNodeTitle's JSDoc, since this is now the single chokepoint for all title derivation and the next person reading it won't have the old comment to reference.

Nitpicks

• Nit: packages/desktop-app/src/lib/types/node-viewers.ts — the interface doc comment references "computeTabTitle in tab-title.ts" by filename; fine as internal documentation, just flagging that if tab-title.ts is ever renamed this comment silently goes stale (no enforcement either way, purely cosmetic).
• Nit: task-node-viewer.svelte still declares an unused onNodeIdChange?: (_nodeId: string) => void prop type "for future navigation features" — harmless, consistent with pre-existing style in that file, not introduced by this commit.

Testing Assessment

Independently re-ran the five test files most relevant to this commit (plugin-registry.test.ts, core-plugins.test.ts, navigation.test.ts, pane-content-hydration.test.ts, tab-persistence-service.test.ts): 215 tests, 0 failures, consistent with the author's reported clean targeted run (the exact count differs slightly from the 257 cited in the task brief, likely due to file-set selection, not a discrepancy of concern). svelte-check shows 7 pre-existing errors, none in files touched by this diff (calendar components, code-block-node, table-node, test setup mocks) — confirmed by checking out 314b709f and observing the same errors exist there. This is not a regression.

The deleted test blocks (updateTabTitle, updateTabContentAndTitle in navigation.test.ts; "date node title migration" in tab-persistence-service.test.ts) correctly track the deleted production code — no orphaned assertions, no coverage silently lost without a replacement rationale (the migration test's behavior is now correctly untestable because the behavior itself no longer exists — titles aren't recomputed on load because they're never read from load).

Recommendation: APPROVE

This commit is a clean, well-reasoned resolution of the prior review's Important finding (b), and it resolves it by removing the vulnerable pattern entirely rather than guarding against misuse of it — a stronger and more idiomatic fix given Svelte 5's rune model. The architectural conversion (push-based title mutation → pure derivation) is applied consistently across all 7 viewer components with no stragglers, the two production-code deletions in navigation.ts and tab-persistence-service.ts are correctly justified by the new invariant (titles are never persisted or pushed, only computed on read), and all touched test suites pass cleanly with no unrelated regressions. The one gap worth a follow-up (direct test coverage for computeTabTitle's own branches) is minor given the function it delegates most of its logic to is well-tested, and does not block merge.

[malibio]

Re-review of a4b3ced (GitHub blocks self-approval on this account, so filing as COMMENT with an approve recommendation): prior Important finding (b) is resolved by fully removing the push-based title mechanism rather than guarding it — stronger fix than suggested. All viewer call sites migrated consistently, deleted tests correctly track deleted code, targeted test runs clean (215/215), no regression in svelte-check. One minor gap noted (computeTabTitle lacks direct unit tests) but not blocking. Recommendation: APPROVE — see full review comment above for details.

[malibio]

Addressed review feedback (commit c04f8f5)

Both minor suggestions from the re-review of commit a4b3ced:

• ✅ Added direct unit tests for computeTabTitle() (tab-title.test.ts, 7 tests) — previously only indirectly covered via getNodeTitle's tests. Covers tab-type gating (settings/placeholder/node-with-no-content), the getNode lookup and its not-found fallback, delegation to pluginRegistry.getNodeTitle, the fallback-to-tab.title path, and formatTabTitle truncation/header-stripping.
• ✅ Restored the "node.title is backend-computed" assumption as an explicit comment on pluginRegistry.getNodeTitle(), pointing at NodeService::compute_title in the Rust core — this was previously explained in a comment on the base-node-viewer.svelte code this PR deleted, and had become implicit.

No other findings from either review round remain open.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: Both changes are additive (new test file, one restored comment) with no logic changes to already-reviewed code. svelte-check, eslint, and the full targeted test set (tab-title.test.ts + plugin-registry.test.ts, 103 tests) pass cleanly.