Fix: Progress listener torn down before background import completes

[malibio]

Summary

• Fixes status bar getting stuck on "Creating 30 collections..." during import

Problem

The import progress listener was being unsubscribed immediately after importDirectory() returned. However, the backend returns after Phase 1 (parsing) while Phase 2 (database operations - steps 5-9) runs in a background tokio::spawn task.

Changes

• Remove automatic listener teardown from importDirectory()
• Update app-shell to only unsubscribe when step 9 (complete) is received
• Move collection refresh to step 9 handler (after background completes)

Test plan

• Import a folder of markdown files
• Verify all 9 steps are shown in status bar
• Verify completion message shows "Imported X files (Y nodes) in Z.Xs"

🤖 Generated with Claude Code

[malibio]

Code Review Summary

Verdict: APPROVE

This PR correctly addresses the issue where the status bar was getting stuck on "Creating 30 collections..." during import. The root cause was the progress listener being torn down in finally block after importDirectory() returned, but before Phase 2 (background database operations) completed.

Analysis of the Fix

The fix is architecturally sound and follows the right pattern:

1. Import Service (import-service.ts): Removed automatic teardownProgressListener() from the finally block, with clear documentation that Phase 2 runs in background.

2. App Shell (app-shell.svelte): Moved listener lifecycle management to the caller with proper cleanup:

   • Unsubscribes only when step 9 (complete) is received
   • Unsubscribes on error (both Phase 1 failures and exceptions)
   • Moved collection refresh to step 9 handler (correct timing - after background completes)

Findings

No Critical Issues

The implementation correctly handles all code paths for listener cleanup.

Suggested Improvements

[Improvement] /packages/desktop-app/src/lib/components/layout/app-shell.svelte (lines 194-260): Consider adding a timeout fallback for orphaned listeners

If the backend crashes or step 9 is never emitted (e.g., panic in tokio::spawn), the progress listener will remain subscribed indefinitely. While unlikely in practice, a defensive timeout (e.g., 5 minutes) would ensure cleanup:

// Optional defensive timeout
const importTimeout = setTimeout(() => {
  if (unsubProgress) {
    log.warn('Import progress listener timed out - cleaning up');
    unsubProgress();
    unsubProgress = null;
  }
}, 5 * 60 * 1000); // 5 minute timeout

// Clear timeout in step 9 handler and error handlers
clearTimeout(importTimeout);

Rationale: Follows defensive programming principle to handle unexpected failure modes, though current implementation is acceptable given the low probability.

[Nit] The collectionsData import is already at the top of the file (line 21), so the dynamic import on line 226 is unnecessary:

// Current (line 226):
const { collectionsData } = await import('$lib/stores/collections');

// Could be simplified to just use the existing import:
await collectionsData.loadCollections();

However, this is a minor style inconsistency - both work correctly.

Security

No security concerns - this change only affects UI progress reporting.

Testing Recommendations

Manual testing as outlined in the PR description is appropriate:

• Import a folder of markdown files
• Verify all 9 steps are shown in status bar
• Verify completion message shows "Imported X files (Y nodes) in Z.Xs"

Conclusion

The fix correctly addresses the listener lifecycle issue. The code is well-documented with clear comments explaining the Phase 1/Phase 2 architecture. The suggested timeout improvement is optional and can be added in a follow-up if desired.

---

Reviewed by: Claude Code (Pragmatic Quality Framework)

[malibio]

Code review complete - this fix correctly addresses the listener lifecycle issue. Ready for merge pending manual testing of the 9-step import flow.

[malibio]

Review Feedback Addressed

✅ Addressed (1 recommendation)

• Nit: Removed unnecessary dynamic import of collectionsData - it's already imported at the top of the file

⏭️ Skipped (1 recommendation)

• Timeout fallback for orphaned listeners: While reasonable in theory, this was skipped because:
  • Low probability edge case (backend crash before step 9 emits)
  • Listener is lightweight and will be cleaned up on next import or app restart
  • Adding timeout logic adds complexity for marginal defensive benefit
  • Can revisit if this becomes an issue in practice

Summary

• 📝 Commits: 1
• 🧪 Tests: Quality checks passed
• ✅ Ready for merge