[malibio]

Summary

import_markdown_file / import_markdown_files broadcast import-progress to every window with bare app.emit, and the payload had no database id, so concurrent imports (or a future second window) couldn't be told apart.

• GrpcClient::import_client now returns (ImportClient, Option<String>): the client plus the database id its x-ns-database-id header names. Both are read under one lock, so a database switch mid-call can't pair a client with the wrong id.
• ImportProgressEvent gets a database_id field. It is left out of the payload when the import targets the daemon's default database. Both commands now send progress through window_routing::emit_routed using a shared forward_progress helper.
• The TS ImportProgressEvent type gets an optional database_id.

Not reachable today: there's one window and imports run one at a time. This gets routing right before multi-window or concurrent-import work ships.

Tests

• forward_progress_reaches_only_the_window_showing_the_import_database: mock app with two windows pinned to db-1 and db-2. An import into db-1 reaches only db-1's window, and its payload carries database_id: "db-1".
• progress_event_omits_database_id_for_default_database
• cargo test -p nodespace-app --lib commands::import: 15 passed. bun run test: 5313 passed.
• Pushed with --no-verify at the maintainer's request (a fix for a timing-flaky test is pending), so the full pre-push gate did not run.

🤖 Generated with Claude Code

[malibio]

Review Type: Initial Review

Requirements Check:

• ✅ Route import-progress through window_routing::emit_routed: both import_markdown_file and import_markdown_files now go through the shared forward_progress helper. No bare app.emit("import-progress", ...) remains.
• ✅ Add database_id to ImportProgressEvent: added as Option<String> with skip_serializing_if. The TS mirror in import-service.ts is updated to database_id?: string.
• ✅ Id is consistent with the client that made the request: GrpcClient::import_client now returns the client and active_database_id from a single read-lock. A concurrent set_active_database therefore cannot pair one database's client with another database's id. Both callers are updated and nothing else calls import_client().
• ✅ Tests: a routed two-window test shows progress reaches only the pinned window and carries the id. A serde test shows the field is omitted for None.

Code Review Findings:

1. 📁 packages/desktop-app/src-tauri/src/commands/import.rs:143-167 (behavior for database_id: None) — 🟢 Suggestion
   I checked the case you flagged: the id is None while the window is pinned to a concrete id. It can happen, and it happens on every launch. databaseStore.load() (database.svelte.ts:204-212) calls pin_window_database(resolved) with a concrete id (the launch-requested id, the remembered id, or the daemon default). It never calls set_active_database. The only set_active_database call is in switchTo() (database.svelte.ts:397). So until the user first switches, GrpcClient.active_database_id is None, the import has no header, and the window is pinned to e.g. "default-db-id". emit_routed(.., None) then takes the focused-window path, falling back to the lowest label, so progress still reaches the single window. That is correct today.
   It also means the None pass-through is the right choice. Resolving None to the daemon default's concrete id would make things worse: when the pin is a remembered non-default id, the event would be dropped. Keep it as-is. Consider a one-line note in the forward_progress doc that None routes to the focused window, not the window pinned to the default database. With multiple windows, "default database" and "focused window" can diverge. This matches the node-event watcher, which routes on the same active_database_id, so the behavior is at least consistent (principle of least surprise).

2. 📁 packages/desktop-app/src/lib/stores/database.svelte.ts:204-212 (pre-existing, outside this diff) — 🟡 Important (file separately, do not block this PR)
   While tracing finding 1 I found that at startup the window pin and the gRPC routing can disagree. When resolved is a remembered or launch-requested id that is not the daemon default, the window is pinned and the switcher shows database X. But GrpcClient still has no routing header, so node, import, embeddings and agent requests go to the daemon default. From the code it looks like a restored non-default selection never reaches the Rust routed clients until a manual switchTo. This PR does not cause it. The None → focused-window fallback actually keeps import progress visible in that state. It is still a correctness gap in the data plane, and it becomes a routing gap once multiple windows exist. Recommend a follow-up issue: load() should call set_active_database(resolved) before pinning, or pin_window_database and set_active_database should become one operation so they cannot drift (single source of truth).

3. 📁 packages/desktop-app/src-tauri/src/commands/import.rs:190-195, 248-253 — 🟢 Suggestion
   Semantics change during a mid-import database switch: if the user switches the window to another database while an import with Some(id) streams, emit_routed finds no window pinned to the old id and drops the remaining progress events. Before this change it broadcast them. The modal's progress bar would freeze, but the invoke still resolves with the final BatchImportResult, so the outcome is still reported. Arguably this is the intended behavior (the window no longer shows that database) and it is a narrow case. Worth knowing, not worth code today (YAGNI).

4. 📁 packages/desktop-app/src-tauri/src/commands/import.rs:659-718 — 🟢 Suggestion
   The new tests cover the Some(id) routed path and the serialization of None. They do not cover the path described in finding 1 that production actually takes today: pins present, id None, one window gets it. That behavior already has tests in window_routing.rs, so this is optional. A short test of forward_progress(&handle, .., None) with one pinned window would lock in the invariant that import progress always reaches the lone window. That is the one regression users would notice.
   Nit: the two-window test mostly repeats emit_routed_reaches_only_the_window_pinned_to_the_database from window_routing.rs. What it adds is the payload assertion (database_id, step_name), which is good. You could trim it to that payload check plus one window to reduce duplication (DRY in tests).

5. 📁 packages/desktop-app/src/lib/services/import-service.ts:91-92 — 🟢 Suggestion
   database_id is added but nothing on the frontend reads it. The modal subscribes through importService.onProgress with no filtering. That is fine per the issue, since the payload now lets a future listener tell imports apart. But a consumer that compares database_id === databaseStore.activeDatabaseId would silently fail for default-database imports: the field is absent while activeDatabaseId holds the concrete default id. The TS doc comment already says "absent when it targets the daemon's default database", which is enough warning.

No security, lint-suppression, backward-compat, or issue-number-in-comment problems. The change is small, atomic, and matches the existing emit_routed pattern the watcher uses.

Recommendation: APPROVE
It is a net improvement and a correct, minimal implementation of the issue's suggested fix. The fallback semantics hold for every state reachable today, including None with a pinned window. Finding 2 is a pre-existing problem that should get its own issue, not block this PR.

[malibio]

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

[malibio]

Review addressed (17f5580)

✅ #1 (doc note): forward_progress now says a None id goes to the focused window, not to "the default database's window".
✅ #4 (test): new forward_progress_without_database_id_reaches_the_pinned_window covers the path production uses today: one pinned window, no routing header. The two-window test stays because it checks the payload's database_id.
📋 #2: confirmed and filed as #3058. load() pins the window but never calls set_active_database, so after a restart onto a non-default database, requests go to the default database.
⏭️ #3 (switching databases mid-import drops later progress): skipped. This is the intended routing, and the command still returns the final result.
⏭️ #5 (frontend database_id filter): skipped. Nothing reads the field yet, and the TS doc already notes that it's absent for default-database imports.

Tests: cargo test -p nodespace-app --lib commands::import, 16 passed.

Re-review decision: NO RE-REVIEW NEEDED. The changes are one doc comment and one test, with no logic changes.