Repository navigation
fix(library): a failed import is reported, and the library is re-read either way - #147
Merged
Merged
Conversation
… either way Fixes #146. A failed import used to produce nothing at all — no error, no row, and a panel that still showed its empty state, which reads as "the upload worked and the app is not showing it". #37 was closed with exactly that report and nothing in the code could have told that user what went wrong. Two halves, both needed: **The failure was discarded before it could be shown.** `SourcePanel` calls the store and ignores what it returns. The store reports failure by *returning* `{ success: false, error }` — the IPC layer already catches and describes it — so an unchecked return value makes a failure indistinguishable from a success. `handleFileUpload`, `handleUrlImport`, `handleTextPaste` and `handleNoteImport` all did this. **The list was only re-read when the import succeeded.** `KnowledgeService` inserts the source row and *then* indexes it, so a failure at the indexing step leaves a row with `status: 'failed'` — and `DocumentList` already has the feedback for it, an alert saying the document failed to embed. It never fired, because the list was never re-read. A failure *before* the row exists (an unsupported type, a corrupt or scanned file that extracts nothing) has no row and so needs the message to say so. Now one rule covers every entry point: an import attempt always ends by re-reading the library and recording the reason, and the reason reaches the reader as a toast. The rejection path re-reads too, deliberately: an IPC call can fail after the main process already wrote the source row, so the failure path cannot assume that nothing changed. Not in this PR: recording an attempt that failed before its source row existed (there is still no row for an unparseable file — that is #95's ingestion record), and any retry affordance (#95). The `hasEmbeddingModel` gate that disables `+` is untouched; after #134 the built-in local backend makes it effectively always open. One behaviour note: the empty-note case used to `alert()` from a branch that could not be reached — the store returns `{ success: false }` rather than throwing, so the `catch` never ran. It now shows the same message as a toast, like every other import failure, and names the note instead of printing `Note note_123 not found`. Verified: `npm test` 303 pass (6 new in `test/knowledgeStore.test.ts`, which drive all four entry points through the store with a stubbed `window.api`), `npm run typecheck`, `npm run check:design`, `npx eslint` on the changed files (0 problems), `npm run build`. `tsconfig.test.json` now also includes `src/preload/index.d.ts`: importing renderer code means typechecking it, and that file is where `window.api` is declared — without it a test cannot import a store that legitimately uses the preload global. Not run: `build:unpack` + `smoke:packaged`. Neither main nor preload changed; the packaged checks are in CI for this PR and were run in #145 for the last main-process change. Not hand-verified: the toast itself, which needs the GUI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #146. A failed import used to produce nothing at all: no error, no row, and a panel that still showed its empty state — which reads as "the upload worked and the app is not showing it".
Two halves, both needed
The failure was discarded before it could be shown.
SourcePanelcalls the store and ignores what it returns:The store reports failure by returning
{ success: false, error }— the IPC layer already catches and describes it — so an unchecked return value makes a failure indistinguishable from a success.handleFileUpload,handleUrlImport,handleTextPasteandhandleNoteImportall did this.The list was only re-read when the import succeeded.
KnowledgeService.addDocumentFromFileinserts the source row and then indexes it:A failure at step 4 leaves a
status: 'failed'row, andDocumentListalready has the feedback for it — an alert saying the document failed to embed — but the list was never re-read, so the row never rendered and the alert never fired. A failure at step 2 (unsupported type, a corrupt or scanned file that extracts nothing) has no row at all and needs the message to say so.What changes
importFailed, en + zh).errorstate is now set from a failed result as well as from a thrown error — both are failures, and only the second one used to be recorded.Not in this PR
hasEmbeddingModelgate that disables+— unrelated, and after fix(embedding): enable RAG with the built-in local model and batch local embeddings #134 the built-in local backend makes it effectively always open.Behaviour note
The empty-note case used to
alert()from a branch that could not be reached: the store returns{ success: false }rather than throwing, so thecatchnever ran. It now shows the same message as a toast like every other import failure, and names the note instead of printingNote note_123 not found.Verification
npm testtest/knowledgeStore.test.tsnpm run typechecknpm run check:designnpx eslint <changed files>npm run buildThe new tests drive all four entry points (
pasted text,a file,a URL,a note) through the real store with a stubbedwindow.api, and assert three things per entry point: the failure is reported, the library is re-read (so astatus: 'failed'row can appear), and the reason is recorded. Two more cover the success path clearing an earlier failure, and a rejected IPC call.tsconfig.test.jsonnow also includessrc/preload/index.d.ts. Importing renderer code means typechecking it, and that file is wherewindow.apiis declared; without it a test cannot import a store that legitimately uses the preload global. The test file's header records why.Not run:
build:unpack+smoke:packaged— neither main nor preload changed (CI runs them for this PR, and they were run in #145 for the last main-process change). Not hand-verified: the toast itself, which needs the GUI. To check it by hand: Library → + → Upload file → pick a.zipor a scanned PDF → a red toast naming the file, instead of nothing happening.