Fix tab reset and status bar on database hot-swap#904
Conversation
- Clear all stale tabs instead of resetting to hardcoded "Daily Journal" - Open fresh Today tab so DateNodeViewer sets its own title - Show new database path in status bar after swap Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add onNodeNotFound callback to BaseNodeViewer — auto-closes tabs when the referenced node doesn't exist (stale persisted state, deleted nodes, wrong database) - Fix stale nodes poller stomping on status bar messages by only clearing when transitioning from count > 0 to 0 - Increase success message display from 3s to 5s Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pragmatic Code Review -- PR #904Review Type: Initial Review Requirements Check (Issue #900 Acceptance Criteria)
All acceptance criteria met. Code Review FindingsNo Critical Issues FoundThe change is clean, well-scoped, and addresses a real set of UX issues. The two commits are logically separated (core swap fix vs. stale tab auto-close + poller fix). Suggested Improvements🟡 [Improvement]
This is not a regression from this PR (the edge case existed before), and the database hot-swap flow itself is safe because Verdict: Not a blocker. Document as a known limitation or future enhancement. 🟡 [Improvement] The A simple fix would be to store the timer ID and clear the previous one: let successTimer: ReturnType<typeof setTimeout> | undefined;
success(message: string) {
if (successTimer) clearTimeout(successTimer);
update((state) => ({ ...state, message, type: 'success', progress: undefined }));
successTimer = setTimeout(() => {
update((state) => ({ ...state, message: '', type: 'info' }));
successTimer = undefined;
}, 5000);
},Verdict: Low risk in practice for this PR since database swaps are infrequent, but the pattern is a latent bug. Worth fixing in a follow-up. NitpicksNit: Nit: Architecture & Design AssessmentPositive observations:
Final RecommendationAPPROVE ✅ This PR is a clear net positive. It fixes real UX issues (stale tabs, status bar stomping, broken tab titles after database swap), the code is clean and well-structured, and there are no security, performance, or architectural concerns. The two improvement suggestions above are non-blocking and can be addressed in follow-up work. Reviewed by: Principal Engineer Reviewer (Pragmatic Quality Framework) |
malibio
left a comment
There was a problem hiding this comment.
APPROVE (submitted as comment — GitHub blocks self-approval). Net positive change. All acceptance criteria met. Clean implementation with good separation of concerns. Two non-blocking improvement suggestions noted in the detailed review comment above.
Clear previous timer before setting a new one so rapid successive success() calls don't have the first timer clear the second message. Skipped: closeTab last-tab guard — pre-existing behavior, not introduced by this PR, and database swap always adds a fresh tab. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Fix tab reset on database hot-swap (#900) - Clear all stale tabs instead of resetting to hardcoded "Daily Journal" - Open fresh Today tab so DateNodeViewer sets its own title - Show new database path in status bar after swap Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Auto-close stale tabs and fix status bar message timing - Add onNodeNotFound callback to BaseNodeViewer — auto-closes tabs when the referenced node doesn't exist (stale persisted state, deleted nodes, wrong database) - Fix stale nodes poller stomping on status bar messages by only clearing when transitioning from count > 0 to 0 - Increase success message display from 3s to 5s Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Address review: fix success() timer stomping Clear previous timer before setting a new one so rapid successive success() calls don't have the first timer clear the second message. Skipped: closeTab last-tab guard — pre-existing behavior, not introduced by this PR, and database swap always adds a fresh tab. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Follow-up to PR #903 — fixes issues found during manual testing of database hot-swap:
clearAllTabs()removes all old tabs (they point to nodes in the old database), then opens a fresh Today tab —DateNodeViewermounts from scratch and sets its own titleBaseNodeViewernow callsonNodeNotFoundwhen the referenced node doesn't exist after loading, andpane-content.sveltecloses the tab. Handles stale persisted state, deleted nodes, wrong database. Date tabs are exempt (lazy creation).statusBar.success()clearMessage()every 5s even with count=0, wiping other messages. Now only clears when transitioning from count > 0 to 0.Files Changed
stores/navigation.tsclearAllTabs()layout/app-shell.sveltelayout/pane-content.svelteonNodeNotFound→closeTabdesign/components/base-node-viewer.svelteonNodeNotFoundcallbackstores/status-bar.tsTest plan
bun run test:all— no new failures (baseline: 3586 passed)🤖 Generated with Claude Code